-
Notifications
You must be signed in to change notification settings - Fork 198
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
Fix usage of stale state in legends racefilter #1346
Conversation
Prevents usage of stale state when loading multiple worlds or timelines in a single session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice fix -- could you add a line to the changelog?
@@ -1038,6 +1038,11 @@ dfhack.onStateChange[GLOBAL_KEY] = function(sc) | |||
if sc == SC_VIEWSCREEN_CHANGED and df.viewscreen_choose_game_typest:is_instance(dfhack.gui.getDFViewscreen(true)) then | |||
asyncexport.reset_state() | |||
end | |||
|
|||
-- Reset state when a world or map is loaded to ensure data remains current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why map and not just world? isn't race specific to world?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes can occur between loading the map and opening legends thanks to the open-legends
tool. As fort mode could easily modify historical data, I thought it safest to ensure that path starts with a reset state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change won't catch that specific case -- if a map is loaded, then the race of a histfig is changed (dangerous, btw!), then open-legends
is run, this check won't catch it. I'm not certain that it's a case we really need to address, though. resetting on world load, however, is very important and should absolutely be done here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only reason I'm being picky about this is that maps are loaded and unloaded all the time in adventure mode, and I don't think we should run more than we really have to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally worried about werebeasts changing the race of histfigs, but after some research that doesn't appear to occur. As we aren't particularly worried about the case of open-legends
, removing the reset on map load seems safe to me.
Loading multiple worlds or timelines in legends mode originally did not reset the state, allowing stale data to be used in filtering. This resulted in incorrect results being presented.
This patch resets
racefilter
state on world or map load to ensure the state remains up to date.Map loading is checked to prevent issues with
open-legends
which is able to access legends mode without reloading the world.