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

[WIP] Fixed "Auto-sort infinite loop/game hang #29573" #32876

Closed

Conversation

ipcyborg
Copy link
Contributor

@ipcyborg ipcyborg commented Aug 3, 2019

Summary

SUMMARY: Bugfixes "Auto-sort infinite loop/game hang #29573"

Purpose of change

Fixes #29573

Describe the solution

The sorting loop was still reproducible even after this fix "Zone Sorting Loop #31895".
I have tried to rewrite activity_on_turn_move_loot with the idea of different sorting stages:

  1. Initial stage: determine all possible sources of items in the unsorted zones. Remember all sources in the player activity.
  2. Move player stage: search for the next appropriate source and start to move there.
  3. Move items stage: get all items from the source and move to appropriate destination.

Note that this algorithm relies completely on the activity instance and does not require any cache from zone_manager.
So in theory now it could be safely applied to multiple NPC & player in parallel.

TODO:

  • [Still have to figure out what to do with NPC sorting. Can't really decipher what previous algorithm wanted to do with unloading/reloading NPC off the map. ]

Help would be really appreciated here, thanks.

Describe alternatives you've considered

I have tried to keep the original code intact as much as possible. But it would probably be better to use separate activities instead of stages for the same activity?

Additional context

It will still halt if NPC is blocking the auto move route (with specific message). Quite sure it was the same before the change.
It is completely safe to step away and redo the sort after it.

Also this fix should improve performance of the sorting process.

Not sure why I have to replace player activity after p.set_destination and set it back only after the auto move is finished. Or it is not required?

Tested on the save provided in #29573.

@ghost
Copy link

ghost commented Aug 3, 2019

This is gonna clash with #32730 hugely.
Ive basically removed the sort loot function there and folded it into a new generic multi-activity loop.

I still use the same structure and zone caching however.

If your way is superior, it may be best for me to remove the loot sorting aspect of my PR, so you can get this merged, and then afterwards, fold your new process back into the generic multi-activity function.

Seeking feedback from one of the senior devs on the best route forward on that.

Not sure why I have to replace player activity after p.set_destination and set it back only after the auto move is finished. Or it is not required?

Players ( and NPCs ) have the destination_activity() that automatically starts an activity after a route has been moved to, as far as I can tell, you don't need to do the check for if( p.has_destination() ) { //not there yet, continue moving return; }

because they wont be running the activity code again until they actually arrive.

the p.set_destination( route, act ); p.activity = player_activity();

clearing the activity is done at the start of the current implementation of this function.
whether you do it at the start or after setting the destination, either way will ensure that the NPC or player has no activity code to run anymore, and will instead move on to the automove to destination part, and then restart the activity when they get there.

Every turn that an NPC is doing an activity, the player could be moving the reality bubble, and they could go out of bounds during their activity, the existing code is there to unload them safely, and is required.

@ipcyborg
Copy link
Contributor Author

ipcyborg commented Aug 3, 2019

This is gonna clash with #32730 hugely.

Yes, definitely.

UPDATE:
#32730 is committed, will check this PR again

@ZhilkinSerg ZhilkinSerg added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones labels Aug 3, 2019
@ipcyborg ipcyborg changed the title [WIP][CR] Fixed "Auto-sort infinite loop/game hang #29573" [WIP] Fixed "Auto-sort infinite loop/game hang #29573" Aug 20, 2019
ipcyborg added a commit to ipcyborg/Cataclysm-DDA that referenced this pull request Aug 31, 2019
Added save/load for "coord_set" in "player_activity".
@ipcyborg
Copy link
Contributor Author

Closed by #33710 (rebased).

@ipcyborg ipcyborg closed this Aug 31, 2019
@ipcyborg ipcyborg deleted the fix-29573-auto-sort-loop branch October 17, 2019 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-sort infinite loop/game hang
2 participants