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

Restructure zone view lighting #3813

Conversation

kwvanderlinde
Copy link
Collaborator

@kwvanderlinde kwvanderlinde commented Jan 10, 2023

Identify the Bug or Feature request

Implements #3747

Description of the Change

The main purpose was to improve the performance of personal lighting. The performance itself is mildly improved, though the main benefit is increased responsiveness when switching PlayerViews. There is still room for improvement as personal lights still have a noticeable impact above and beyond normal lights.

The secondary purpose of this change was to enable some different approaches to lights. E.g. I am experimenting with splitting the lumens display out from light blending, which is one of the things that is made easier due to the new Illuminator providing a more complete picutre of the lighting situation.

At the core of this change is the addition of Illuminator and its result type Illumination. An Illuminator holds all areas lit by normal lights, and associates them with their lumens value (this is actually done per sight multiplier since that can change the light areas). It can then produce an Illumination that holds the various lumens levels as unioned areas, from which we can calculate some derivate results such as which areas are lit and which areas are covered in darkness. The ZoneView is then able to use this "global" result and mix in personal lighting information in order to produce a final result.

The caching in ZoneView has also been reworked so that more information can be kept as things change. The main lighting results are cached per IlluminationKey (which is just a sight multiplier for now) and which can be reused for every token involved in a view. Other results are cached per PlayerView. This includes some results that are specific to the current PlayerView being rendered, but now are stored in a Map so that they can still be reused even when changing back and forth between PlayerViews. As a result, view changes no longer require flushing the ZoneView, and correspondingly the zoneView.flush() call has been removed from ZoneRenderer.invalidateCurrentViewCache().

A couple minor changes for things discovered during this work as well:

  • FogUtil.calculateVisibility() is no longer responsible for translating areas according to the visibility origin. That responsibility is now left to the callers, and enables some call sites to be a bit more streamlined then they would otherwise be in terms of managing lit areas.
  • The Direction enum has been removed as only Direction.CENTER was actually used. This also mean we don't have to try and account for it in any caching.
  • The CodeTimer was tweaked so it doesn't repeat ordering numbers when a single timer is encountered multiple times.

Possible Drawbacks

There is a bit more going on, so there is the possibility of missing something. E.g., not flushing illumination results properly. This would appear as lights not appearing, or appearing in the wrong place.

Documentation Notes

N/A (internal change)

Release Notes

  • Mild performance improvement on maps that have many personal lights. Selecting / deselecting tokens will not generally results in long wait times in that case.

This change is Reviewable

At the core of this change is the addition of `Illuminator` and its result type `Illumination`. An `Illuminator` holds
all areas lit by normal lights, and associates them with their lumens value (this is actually done per sight multiplier
since that can change the light areas). It can then produce an `Illumination` that holds the various lumens levels as
unioned areas, from which we can calculate some derivate results such as which areas are lit and which areas are covered in
darkness. The `Illuminator` is also able to add and remove individual areas and will only recalculate the effected
lumens level.

The caching in `ZoneView` has also been reworked so that more information can be kept as things change. The main
lighting results are cached per `IlluminationKey` (which is just a sight multiplier for now) and which can be reused for
every token involved in a view. Other results are cached per `PlayerView`. This includes some results that were
previously specific to the current `PlayerView` being rendered, but now are stored in a `Map` so that they can still be
reused across view changes. As a result, view changes no longer require flushing the `ZoneView`, and correspondingly the
`zoneView.flush()` call has bee removed from `ZoneRenderer.invalidateCurrentViewCache()`.
@kwvanderlinde kwvanderlinde changed the title Refactor/3747 structured zone view lighting Restructure zone view lighting Jan 10, 2023
Copy link
Collaborator

@thelsing thelsing left a comment

Choose a reason for hiding this comment

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

I don't claim that I completely understand ZoneView but the code looks reasonable. I'm a bit surprised that we don't use the direction of a light anywhere. Don't we have light cones?

- Renamed `Illuminator.Node` to `Illumination.Node` and added more documentation.
- Factored out the path extension so we don't duplicate the details of getting a path iterator from a `LitArea`.
Rather than unioning every token's vision, we can instead use the visible area of the view's illumination result. That
already accounts for each token's personal lights and sight multipliers.
@kwvanderlinde
Copy link
Collaborator Author

I don't claim that I completely understand ZoneView but the code looks reasonable. I'm a bit surprised that we don't use the direction of a light anywhere. Don't we have light cones?

That Direction enum isn't even really a direction so much as a statement of where on the token a light should originate, i.e., top-left corner, middle right side, etc. Our cones on the other hand always originate at the center, and then rely on their configured arc and offset angles to pick a direction relative to the token's facing.

Copy link
Contributor

@Phergus Phergus left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 15 of 17 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kwvanderlinde)

@Phergus Phergus merged commit 3c91474 into RPTools:develop Jan 12, 2023
@kwvanderlinde kwvanderlinde deleted the refactor/3747-structured-ZoneView-lighting branch January 12, 2023 18:46
@cwisniew cwisniew added the performance A performance or quality of life improvement label Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance A performance or quality of life improvement
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

4 participants