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.

Sign up for GitHub

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

Jump to bottom

Fix #12301: Update ship current order destination tile when relocating buoys #12303

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

Fix #12301: Update ship current order destination tile when relocating buoys #12303

wants to merge 1 commit into from

Conversation

SamuXarick
Copy link
Contributor

Motivation / Problem

#12301
Relocating a buoy to another tile isn't updating v->dest_tile, sending the wrong tile for the pathfinders.

Description

Fix #12301 Update v->dest_tile when relocating buoys.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@SamuXarick SamuXarick marked this pull request as draft March 15, 2024 14:39
@SamuXarick

This comment was marked as outdated.

Sign in to view
@SamuXarick SamuXarick force-pushed the update-v--dest_tile-when-relocating-buoys branch from 0bf3d92 to 19f7d02 Compare March 15, 2024 15:38
@PeterN
Copy link
Member

PeterN commented Mar 15, 2024 •
edited

I would expect something to hook into CmdBuildBuoy, around waypoint_cmd.cpp:331. i.e. actually update a ship's destination if it needs to be changed.

Might need cached paths to be invalidated too?

@SamuXarick SamuXarick force-pushed the update-v--dest_tile-when-relocating-buoys branch from 19f7d02 to 0160bff Compare March 15, 2024 15:56
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 15, 2024
View reviewed changes
src/order_cmd.cpp Fixed Show fixed Hide fixed
@SamuXarick SamuXarick force-pushed the update-v--dest_tile-when-relocating-buoys branch from 0160bff to 657faf4 Compare March 15, 2024 18:53
@SamuXarick SamuXarick changed the title Fix #12301: Update v->dest_tile when relocating buoys Fix #12301: Update ship current order destination tile when relocating buoys Mar 15, 2024
@SamuXarick SamuXarick marked this pull request as ready for review March 15, 2024 18:57
@Kuhnovic
Copy link
Contributor

I would expect something to hook into CmdBuildBuoy, around waypoint_cmd.cpp:331. i.e. actually update a ship's destination if it needs to be changed.

Might need cached paths to be invalidated too?

That feels the wrong way around to me. The buoy shouldn't be concerned with whoever is using it as a destination. The ship is heading towards the buoy, so it's the ship's job to find a path to it and keep track of the goal.

I find the usage of v->dest_tile a bit strange tbh. Destinations tend to be higher level items such as depots, stations or waypoints. These can then be resolved to a destination tile (or multiple tiles) when a PF call is done. If the position of the destination changes then the next PF run will automatically deal with this.

rubidium42
rubidium42 reviewed Mar 17, 2024
View reviewed changes
src/waypoint_cmd.cpp Outdated
Comment on lines 334 to 337
for (Ship *v : Ship::Iterate()) {
if (v->current_order.IsType(OT_GOTO_WAYPOINT)) {
v->SetDestTile(wp->xy);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please test this PR with ships going to different buoys, and then changing the location of one of the buoys.
As far as I am aware, this is not going to do what you intend it to do in that situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved. Also reduced identation while at it.

@SamuXarick SamuXarick force-pushed the update-v--dest_tile-when-relocating-buoys branch from 657faf4 to 4b0050b Compare March 17, 2024 10:37
@SamuXarick
Fix OpenTTD#12301: Update ship current order destination tile when re…
9121ff0 
…locating buoys

Moving buoys caused current_order to be detected as unchanged, making pathfinders to compute a path to an outdated v->dest_tile.

Properly update v->dest_tile before reaching pathfinders.
@SamuXarick SamuXarick force-pushed the update-v--dest_tile-when-relocating-buoys branch from 4b0050b to 9121ff0 Compare March 17, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubidium42 rubidium42 rubidium42 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Ship becomes lost after moving buoy
4 participants
@SamuXarick @PeterN @Kuhnovic @rubidium42