DFHack / dfhack

Memory hacking library for Dwarf Fortress and a set of tools that use it
Other
1.84k stars 463 forks source link

[tiletypes] reindex pathfinding after altering the map #4743

Closed myk002 closed 6 days ago

myk002 commented 1 week ago

this was done in other parts of tiletypes, but not in the code path called by gui/tiletypes

Bumber64 commented 1 week ago

This is probably out of the scope of this PR, but you could probably replace stuff like

std::string command = commands[loc++];
tolower(command);

with

std::string command = toLower_cp437(commands[loc++]);

and get rid of the plugin's redundant toupper and tolower functions.

You probably don't even need to use our cp437 versions. Comment says the redundant versions exist due to some compile error someone didn't bother to fix, so just fix that.

tatoyoda600 commented 6 days ago

Looks good, just have a question. Would it maybe be better to actually move the reindex_pathfinding code to either the postWrite, paintTile/paintArea, or paintTileProcessing section? That way it's included in the standard processing flow and doesn't depend on it being included in the calling function, since it should always reindex the pathfinding after a map edit

tatoyoda600 commented 6 days ago

I don't know how to leave a code suggestion thing in GitHub, but in this last commit you moved the line to an inaccessible place, right after the return in paintTile (Also it's only in the paintTile function and not in the paintArea one) It should either be inside the postWrites in both paintTile and paintArea, or before the return PaintResult in both functions.