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

CvPlot::setMaxVisibilityRangeCache should set a realistic max #439

Closed
Nightinggale opened this issue Apr 26, 2021 · 1 comment
Closed

CvPlot::setMaxVisibilityRangeCache should set a realistic max #439

Nightinggale opened this issue Apr 26, 2021 · 1 comment
Assignees
Labels
Code Efficiency Make code more efficient

Comments

@Nightinggale
Copy link
Contributor

Nightinggale commented Apr 26, 2021

CvPlot::setMaxVisibilityRangeCache() is called at startup and it determines how far a unit can see in theory. This is then used in the code when scanning the map for units, which might see a certain plot. The smaller this value, the faster the scan.

The issue

The current code loops all promotions and adds up all visibility range changes. This means if a (land) scout can get +2 and a ship an get +2, the code says +4 even though no unit can get more than +2. This can easily result in a way too high MaxVisibilityRange.

Another issue is that it adds all of the values. If somebody adds a tradeoff promotion or a curse giving -1, then the number can become too low and that can then cause bugs.

Proposed solution

Make an EnumMap of UnitCombat, loop all promotion and calculate max viewing distance for each. EnumMap::getMax() then provides the MaxVisibilityRange.

Should only work on promotions providing > 0 visibility change.

Consider if other sources can affect this other than promotions.

@Nightinggale Nightinggale added the Code Efficiency Make code more efficient label Apr 26, 2021
@Nightinggale
Copy link
Contributor Author

Nightinggale commented May 2, 2021

A bit of debugging reveals the game assumes units can view up to 10 plots, meaning if something updates on a plot, which affects visibility (like moving weather), it updates visibility for units in an area of 21x21 plots, which is 441 plots.

Perhaps the solution is more simple than it appears. When starting a game (including loading), CvGame sets this based on GC.getPLOT_VISIBILITY_RANGE() while pretending no unit can view longer than that. When CvUnit sets its own cache for how long it can see, it tells that to a static CvPlot function, which then increases if needed.

This means it will only check according to the unit with the longest viewing range, which was ever in the game. Loading a game can even reduce this if the longest viewing one died.

Reported viewing range Plots to check Reduction in number of plots to check
1 25 94.33%
2 49 88.89%
3 81 81.63%

Note: checked distance is one more than reported because of tall terrain being visible at one plot further away, like ships detect land at +1 range.

In other words a significant difference in the number of plots to check when using this approach. As the limit increases with units, the risk of it going out of sync seems as close to non-existing as possible.


A critical function to consider for this to work is CvUnit::visibilityRange(). It allows improvements to increase visibility range, hence max improvement visibility boost should be considered. I think it's 0 right now, but even then it should be included or it would be a bug waiting to happen.

@Nightinggale Nightinggale self-assigned this May 21, 2021
Nightinggale added a commit that referenced this issue May 29, 2021
When changing something, which might update visibility (like moving weather), the code loops plots, which
might see the changed plot. This update range is now the longest viewing distance of a unit ever seen
in the game rather than vanilla's guess, which was way too high.

Fixed a few bugs where visibility didn't update correctly (all difficult to trigger)

Reintroduced and added assert checks to verify plot viewing is correct.

CvImprovement split iVisibilityChange into one for units and one for plots (was units only).
No plans with plots (yet?) but the new cache design meant it could be added without performance loss.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Efficiency Make code more efficient
Projects
None yet
Development

No branches or pull requests

1 participant