Generalize the use of layers using new properties and standard enum functionality #4402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rather than maintain separate fields for each layer, the fields `drawables`, `gmDrawables`, `objectDrawables`, and `backgroundDrawables` now packaged into a single map so that the required list can be looked up according to layer. Serialization backwards compatibility requires the original fields to still be maintained, but at runtime only the new `drawablesByLayer` is relied on, even for DTO conversions. Some other simplifications were done in this area, such as: - The fields can be `@NonNull` by ensuring the initializer and `readResolve()` set them. - The copying of the lists in the copy constructor can use the `LinkedList` constructor rather than `Collections.copy()` (`Collections.copy()` does not copy the elements themselves despite what the comments seemed to suggest). A bug fix was applied where zone IDs were only being consistently set for token-layer templates, and not for the others. We also now have `Zone.getDrawnElements(Zone.Layer)` that provides external access to the drawing by layer, rather than keeping four separate methods that require hardcoded layer names due to the method naming. This increased flexibility enabled a similar refactoring in `ZoneRenderer` so that the serparate drawable renderer fields could be handled uniformly in a single map as well.
It is now the responsibility of `ZoneRenderer` to decide whether to render a layer. This keeps the decision local to the current map being rendered rather than being globally applied. In the future we may even be able to use a dedicated rednerer in the `ExportDialog` to avoid any sullying of export screenshots with the regular application rendering.
In some places, all the constants in `Zone.Layer` were being spelled out explicitly. These cases have been changed to use `Zone.Layer.values()` instead. There is one change in behaviour here: for token-layer tokens, the _Change To_ menu now shows all layers, just like the analogous menu for stamps. To support this sort of usage, a new method (`Zone.getTokensOnLayer()`) is responsible for returning all tokens on a provided layer. This generalizes and replaces the following `Zone` methods that require the layer to be part of the method name: - `getTokens()` - `getGMStamps()` - `getStampTokens()` - `getBackgroundStamps()` In a similar vein, these methods were removed from Token in favour of directly checking against the result of `getLayer()`: - `isToken()` - `isStamp()` - `isOnTokenLayer()` - `isGMStamp()` - `isObjectStamp()` - `isBackgroundStamp()` The equality checks will later be replaced by more granular checks against layer properties.
In the _Map Explorer_ window's 18in, `OBJECTS` key is now `OBJECT` to enable a future change to lookup by layer name. The **Export Screenshot** dialog used the same key and has likewise been updated. Also in the **Export Dialog**, the field name `LAYER_HIDDEN` is now `LAYER_GM`, with corresponding changes in the `ExportDialog` class.
By moving away from an enum, it is now possible to create `ExportLayers` instances dynamically based on whatever `Zone.Layer` constants exist, and we now do exactly that. This does not address the GUI aspect of `ExportDialog`. If new layers are defined, it will still be necessary to manually add checkboxes to the dialog view in order to control the layers.
Now that `View` is no longer an enum, we can create instances dynamically based on whatever `Zone.Layer` constants exist. The GUI will naturally include any new layers defined in the future.
Now that `View` is no longer an enum, it is possibly to create instances dynamically according to the `Zone.Layer` constants that exist. Any new non-token layers will be included in the GUI, but more particular additions will require some updates to this logic. Alonside that change is a fulfillment of a long-standing TOOD note. The `View` and `TokenFilter` are more coupled now, with `TokenFilter` providing all filtering functionality and configuration, while `View` provides a display name and acts as a lookup key. The `GMFilter`, `ObjectFilter`, and `BackgroundFilter` have all been merged into a single `AdminLayerFilter` that enforces GM and layer checks. The other filters (`PlayerTokenFilter`, `NPCTokenFilter`, and `LightSourceFilter`) remain essentially as they are, just updated for the new system and for consistency. Related to that is some minor refactoring: - The `View.isRequired` flag is gone now as it was always `false`. - The population of `viewMap` was moved out of `TokenFilter` and into `updateInternal()`. This allows coalescing most of the `updateInternal()` loops into a single small loop responsible for populating `viewMap` and `currentViewList`. This also allows us to make the view lists unmodifiable to ensure no one else is messing with the model's internal state. - The `TreeModelEvent` created for structure changes does not need the children and indices parameters to be provided. Removing them avoids any need to keep around static `View` instances.
This new method is `Zone.getTokensForLayers(Predicate, boolean)`. Unlike `Zone.getTokensOnLayer(Layer)`, this method allows an arbitrary predicate to be applied to layers in order to decide which tokens to return. It is a gneeralization of `getTokensOnLayer()` that allows lookup from any number of layers that match the predicate. In practice, this predicate will be built from layer properties so that the lookup is kept abstract and general.
This new method is a convenience for components to get a default layer without having toe rely on a specific one.
This new method is a convenience for components to get a default layer without having to rely on a specific one.
This new method provides a common place to map strings to `Layer` constants. It allows for the use of `"HIDDEN"` in addition to the constant names, but otherwise behaves exactly like `Layer.valueOf()`.
This new method acts as a property of layers, stating whether or not the layer holds tokens. In order to return true, the layer must contain tokens as opposed to stamps. The property should be used in situations where the nature as tokens is important, rather than - for example - the ability to see or pathfind.
This new method acts as a property of layers, stating whether or not thelayer holds stamps. The properties should be used in situations where the nature as stamps is important, rather than - for example - as a negative filter excluding tokens. Most uses of `Token.isStamp()` have been replaced by this new method.
This new method acts as a property of layers, stating whether or not tokens on the layer should pathfind (when enabled). This property - as opposed to `isTokenLayer()` or `!isStampLayer()` - should be used when checking whether pathfinding should be pursued.
This new method acts as a property of layers, stating whether or not tokens on the layer should being able to have their vision calculated and contribute to FoW.
Also add `Token.guessShape()` to do the guessing consistently.
Coalesce GM and non-GM cases in DrawPanelTreeMode.updateInternal() using isPlayerLayer()
For now this is parallel to `isStampLayer()`. However, this is dedicated to deciding whether to treat the token as a marker and avoids convolving the concept with the choice of stamp tooling.
|
I know the TokenFilter class and subclasses predate this change, but while there are changes in those classes, it would be nice to change the method name to test() rather than accept(), since with the addition of functional interfaces introduced in 8 (yeah this class also predates that) test() implies predicate and accept() implies a consumer. So while making this change it would be nice to clean that up |
This makes `TokenFilter` look less like `Consumer<>` and more like `Predicate<>` which it is analogous to.
cwisniew
approved these changes
Nov 16, 2023
thelsing
approved these changes
Nov 16, 2023
cwisniew
added
refactor
cwisniew
added
the
code-maintenance
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug or Feature request
Implements #4378
Description of the Change
This PR could be summarized by three main changes.
The primary change is the introduction of properties associated with each token that can be queried to decide on application behaviour. Rather than comparing against specific layers, we can instead questions of the layer to tell us what behaviour it should have. These are all the new layer properties that exist:
isTokenLayer(): indicates that tokens on this layer are conceptually tokens (TOKEN).isStampLayer()indicates that tokens on this layer are conceptually stamps (GM,OBJECT,BACKGROUND).supportsWalker()indicates that pathfinding, including naive movement, is permitted (TOKEN).anchorSnapToGridAtCenter()indicates the resizing operations for snap-to-grid tokens should keep the token's center fixed, otherwise the top-left corner will remain fixed (TOKEN,GM,OBJECT).oneStepKeyDrag()indicates that movement with the arrow keys should immediate "drop" the token (BACKGROUND).supportsGuessingTokenShape()indicates that tokens added to or moved to the layer can have their shape automatically guessed from the token's image via the newToken.guessAndSetShape()(TOKEN).isVisibleToPlayers()indicates that the layer should be rendered in player views (TOKEN,OBJECT,BACKGROUND).isPlayerLayer()indicates that players can have this layer as their active layer (TOKEN).isMarkerLayer()indicates that tokens on this layer can have their notes and portrait displayed as a marker (GM,OBJECT,BACKGROUND).None of this is exposed to users, it is purely an internal representation.
A second change is to avoid explicit enumeration of layers in various cases. Some of these are obvious, where lists of all
Layerconstants are being created instead of using.values(). Others are sneakier, such as the existence of distinctZonemethods for looking up tokens on each layer, and separate fields for drawings on each layer. Such methods and fields have been coalesced in a way that aLayercan be provided as a key to look up the approriate values. In a similar vein, theTokenmethods for checking layers have been removed in favour of using the new layer properties, or in the worst case, in favour of equality checks againstLayerconstants.Finally, some work was done on those GUI elements that depend on layers. The most extreme of these is
TokenPanelTreeModelthat has been largely reworked to be simplified in the presence of the new layer attributes. Also theDrawPanelTreeModelwill display any and all layers automatically.ExportDialogis in an midway state - the class logic can handle any layers, but the.formfile requires updates to actually make it functional for new layers.The remaining direct uses of
Layerconstants are inZoneRenderer(where the current rendering order is too nuanced to have an obvious generalization) and thegetToken()function (which usesTOKENandGMas defaults despite them not otherwise sharing any behaviour in common).Possible Drawbacks
The "Change To" menu in the right-click menu on the Token-layer now includes the Token layer where it did not before. This matches the behaviour for stamps, but is not ideal.
Documentation Notes
N/A
Release Notes
This change is