Clean up HaloRenderer, with minor fixes #5855
Merged
+349
−497
Conversation
This file contains hidden or 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
Really only `CompositeHaloMiniShapeKey#equals()` was wrong as it mixed up keys 5, 6 with keys 3, 4. But fixing it is simpler even than modifying the logic: just remove `#equals()` and `#hashCode()` as these are provided by default for records. Did the same for `CompositeUnitPolygonShapeKey` and `CompositeUnitShapeKey`.
- `log` field was unused - `token`, `grid` parameters for `paintHaloPart()` were unused - Some unnecessary variable initializers have been removed, and variables are no longer reused between unrelated sections of methods. - `String.format()` is not necessary innside `CodeTimer.start()` and `CodeTimer.stop()` calls - Use a foreach loop on a `.reversed()` list rather than manually consuming a `ListIterator` in reverse - Inject the `Campaign` into `HaloRenderer` - Do not check if a token has certain topology before using it, as it will be `null` anyways (which we expect).
These had nothing to do with a given `HaloPart` and could have been made static where they were. But now any `HaloShapeType` can be directly checked. - `HaloPart#isAngleBasedShape(HaloShapType)` => `HaloShapeType#isAngleBased()` - `HaloPart#isPolyongalShape(HaloShapType)` => `HaloShapeType#isPolygonal()` - `HaloPart#isGeometricShape(HaloShapType)` => `HaloShapeType#isGeometric()` Also add one more such method: `HaloShapeType#isTransformable()`. This is used for halo syntax to check if parameters like `rotate`, `offset`, etc., should be respected.
For `haloPartMap` and `haloFacingAngle`, this is a simple matter of just not setting the fields and instead passing the values as parameters to `renderHaloPart()` and elsewhere. This actually exposes a potential NPE weakness in the current implementation in case there is no parent `Halo`, as is the case for legacy halos. This will be tightened up later. For `haloScaleFactorMap`, we actually need to remember the scale factor in one place to use it later during the reverse halo loop. For this reason, we introduced the `RenderablePart` record that contains all information needed to render a specific `HaloPart`, including the scale factor and other fields. For `haloLineWidthPreference`, we combine this with the halo's own line width quite early, and also include it in `RenderablePart` so that the effective line width calculation does not need to be duplicated. We also lifted the calculation of the halo shape type up into `RenderablePart` so we can calculate it once early on and then reuse the results. This includes using the default shape when the part's shape is `null`, and converting `TOKEN` to either `CIRCLE` or `GRID`. Similarly, we lift the calculation of the facing angle out of the reverse loop and make it part of `RenderablePart`. Only the `haloClipArcByCircleShape` field remains in this regard, but avoiding it requires further refactoring.
The core of this commit is splitting the calculation of the halo shape in `getHaloGeometricShape()` from the calculation of the special mask used for arcs. To avoid excessive duplication, this required removing `transformHaloShape()` and replacing it with a more generic `AffineTransform`-returning `buildTransform()` that takes similar parameters but does not itself transform a shape. Callers can easily apply this transform with no increase in cognitive complexity. During rendering, the mask is now injected as a method parameter to `paintHaloPart()`, freeing that method from deciding what to use as a mask. With these changes, we no longer need the `haloClipArcByCircleShape` field, avoiding any possible issues with state bleeding over from one part to the next. We have also lifted the calculation of the unit shapes up into `renderHaloPart()`. This makes it clear that the unit shape is only useful for geometric shape types (`CIRCLE`, `ARC`, `PIE`, `CHORD`, `TRIANGLE`, `SQUARE`, `POLYGON`, and `STAR`) and the same unit shape is used for mini shapes or regular geometric shapes.
The results are cached in `haloUnitShapeMap` anyways. Also change `HaloRenderer#getUnitPolygonShape()` to only care whether it is building a star polygon or not, and to not accept a full `HaloShapeType` since it can't handle most of them.
kwvanderlinde
added
the
code-maintenance
cwisniew
approved these changes
Nov 15, 2025
github-project-automation
bot
moved this from Awaiting-Review
to To-Be-Merged
in MapTool 1.19
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Follow-up to #5786, to help with conversions required in #5811
Description of the Change
Refactors
HaloRendererto make a future translation to render instructions more straightforward. The main goal here is avoiding the use ofHaloRendererfields to pass data around internal methods for individual halo parts. Instead, method parameters are now used to exchange data, with mutableHaloRendererfields only used for caching potentially expensive results.Most of this was accomplished by building a list of the new
RenderablePartrather than assigning to fields or putting the data into maps for later lookup. This list is directly iterated over during rendering, passing the data along where it is needed. Some of the nested calculations (checking fornullto indicate default token shape or line width, and loweringTOKENshapes toCIRCLEorGRID) have also been moved earlier so they can also be included inRenderablePartto avoided duplicating this logic later on.The
haloClipArcByCircleShapefield was a bit more difficult without duplicating logic as some methods have multiple responsibilities. It's purpose was to be used for a special clip inpaintHaloPart()that applies only toARCshapes, and was calculated ingetHaloGeometricShape()using the sameAffineTransformas the main shape being returned. The field acted as a side channel to get this secondary result to where it is used. But two parallel changes have been made to avoid this:transformHaloShape()was replaced withbuildTransform(), andgetHaloGeometricShape()withgetHaloGeometricTransform(). The new methods only build and return anAffineTransform- they don't perform the transformations as the originals did. This lets the callers reuse theAffineTransformif there is more than one shape it should be applied to. This allowsrenderHaloPart()to transform the halo shape and the mask shape in the same way, without needing the field to act as a side channel.paintHaloPart()no longer decides whether to usehaloClipArcByCircleShapeorpaintShapedepending on the current shape. Instead, it accepts anAreato fill this role, with its callerrenderHaloPart()providing the mask based on the changes in (1).I also removed the
haloUnitPolygonShapeMapcache since these results will be cached inhaloUnitShapeMap. Although this does mean there will be some increased calculation when first rendering several halos of one shape with different angles, the effect is very minor.One very minor bugfix is part of this (no idea if this actually manifests in a meaningful way):
CompositeHaloMiniShapeKey#equals()mixed up a couple of keys, making the results inacurrate. This was fixed by removing theequals()andhashCode()methods, using the default ones provided for all records. The same was done for the other record types inHaloRenderer.Possible Drawbacks
Should be none
Documentation Notes
N/A
Release Notes
N/A
This change is