Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unloading: extract transportee choice to a function #1740

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sprunk
Copy link
Collaborator

@sprunk sprunk commented Oct 27, 2024

No logic change.

@sprunk sprunk added the refactor Internal code cleanup; paying off technical debt; janitorial work. label Oct 27, 2024
return;
}
const auto transportee = GetTransporteeFromUnloadOrder(c, transportees);
if (transportee == nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

if (const auto* transportee = GetTransporteeFromUnloadOrder(c, transportees); transportee) {

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transportee is used a lot afterwards though (from line 1928/1927 until 1972/1971), its scope is the whole rest of the function and not limited just to this check.

@saurtron
Copy link
Collaborator

saurtron commented Dec 9, 2024

Tested this and seems to work fine, also, regarding your question in code: UnloadLandFlood() has just FinishCommand here, is the difference meaningful?

Right now, most of the code when faced with nothing to unload will just do StopMoveAndFinishCommand. Check for example ExecuteUnloadUnit, or ExecuteUnloadUnits. That case is a bit different (since it's for when it does have transportees but an unload order with bad id), but not sure it's going to make a big difference.

Also, at least in bar or z-k I'm not sure anyone actually uses transportUnloadMethod=2 (UNLOAD_LANDFLOOD) where FinishCommand is used instead of StopMoveAndFinishCommand. Also don't see the engine setting that itself but I could be overlooking something.

I also tried changing to FinishCommand() when having an empty queue, (since I couldn't test the case where it has an unload order with an id that's not in the queue so easily... seems to be a pretty niche case too), and for the life of me I couldn't see any difference, but could be because of something else, since a lot happens after/during StopMoveAndFinishCommand/FinishCommand. Usually, if the queue is empty, then Idle() will be called, otherwise it's going to go to the next command anyways, so the difference must be minimal, but it could affect the blending to the next command/idle in some way.

It could also be conflicting with orders to separate a bit from the unload pos. I could see in some cases the transport lands on top of the unit, while others it will go a bit further away, but couldn't correlate that to either of those two being used :P... also in any case the good one would be FinishCommand but can't really tell without a deeper investigation, and the code uses the other one most of the time so I guess there's a reason but couldn't really figure out for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Internal code cleanup; paying off technical debt; janitorial work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants