Minor Fixes #296
Minor Fixes #296
Conversation
…sion from the same agent.
Reviewer's Guide by SourceryThis pull request implements several minor fixes and improvements to the game's mission system, inventory management, and manufacturing process. The changes address issues with mission offers, inventory capacity, and production efficiency calculations. Updated class diagram for RamMethods and related classesclassDiagram
class RamMethods {
+Calculate(args: Call_InstallJob, bpRef: BlueprintRef, pChar: Character)
}
class BlueprintRef {
+productType(): Type
+GetME(): float
}
class Character {
+GetSkillLevel(skill: EvESkill): int
}
class Type {
+productionTime(): float
+productivityModifier(): float
}
RamMethods --> BlueprintRef
RamMethods --> Character
BlueprintRef --> Type
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @beelitzs - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more detailed commit messages for each fix to improve traceability and understanding of changes.
- The PR title 'Minor Fixes' doesn't accurately reflect the scope of changes. Consider a more descriptive title that summarizes the main improvements.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| capacity = PyRep::FloatValue(call.byname.find("capacity")->second); | ||
| m_self->SetAttribute(AttrCapacity, capacity); // The client is telling us the calculated capacity, lets use it to avoid manually calculating it. |
There was a problem hiding this comment.
🚨 suggestion (security): Consider validating the client-provided capacity value
While using the client's calculation saves server-side computation, it's important to validate the input to prevent potential security risks. Consider adding a sanity check or setting a maximum allowed capacity.
const float MAX_CAPACITY = 1000000.0f; // Adjust this value as needed
float clientCapacity = PyRep::FloatValue(call.byname.find("capacity")->second);
float validatedCapacity = std::min(std::max(clientCapacity, 0.0f), MAX_CAPACITY);
m_self->SetAttribute(AttrCapacity, validatedCapacity);
There was a problem hiding this comment.
Based on my experience so far, my server and client already correctly calculates the capacity (expanded cargo holds work and hauler skills increase cargo space; I've filled a hauler's cargo many times). This seems like it might not be needed. It's also a bad idea to trust what the client says - the server is responsible for calculating ship attributes.
Can you elaborate a little more on why this change was made?
There was a problem hiding this comment.
Hi @charles-m-knox! 👋
Only authors and team members can run @sourcery-ai commands on public repos.
| @@ -166,6 +167,7 @@ void MapData::GetMissionDestination(Agent* pAgent, uint8 misionType, MissionOffe | |||
| _log(AGENT__ERROR, "Agent::GetMissionDestination() - no station found within 1 jump." ); | |||
| return; | |||
| } | |||
| sysList.erase(sysList.begin() + randomIndex); // If we have searched a system already then do not try it again | |||
There was a problem hiding this comment.
suggestion: Add a check for empty sysList to prevent potential issues
Consider adding a check to handle the case when sysList becomes empty after erasing all elements. This could prevent potential infinite loops or undefined behavior.
| sysList.erase(sysList.begin() + randomIndex); // If we have searched a system already then do not try it again | |
| if (!sysList.empty()) { | |
| sysList.erase(sysList.begin() + randomIndex); | |
| if (sysList.empty()) { | |
| _log(AGENT__ERROR, "Agent::GetMissionDestination() - all systems searched, none suitable."); | |
| return; | |
| } | |
| } |
| capacity = PyRep::FloatValue(call.byname.find("capacity")->second); | ||
| m_self->SetAttribute(AttrCapacity, capacity); // The client is telling us the calculated capacity, lets use it to avoid manually calculating it. |
There was a problem hiding this comment.
Based on my experience so far, my server and client already correctly calculates the capacity (expanded cargo holds work and hauler skills increase cargo space; I've filled a hauler's cargo many times). This seems like it might not be needed. It's also a bad idea to trust what the client says - the server is responsible for calculating ship attributes.
Can you elaborate a little more on why this change was made?
PR to merge my fork back into staging.
Please squash before merging as I forgot to clean up the branch beforehand this time and continued using it.
Fixes:
Summary by Sourcery
Fix mission completion and system checking issues, enhance inventory capacity handling, and improve manufacturing job calculations by considering skill levels.
Bug Fixes:
Enhancements: