Pass zones to templates so they don't have to look them up #4753
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
kwvanderlinde
force-pushed
the
bugfix/4524-4527-disappearing-and-non-scaling-templates
branch
from
d03f5b5 to
34525c5
Compare
cwisniew
approved these changes
Apr 16, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
Corresponding changes to made to `AbstractDrawing.draw()`, `AbstractDrawing.drawBackground()`, and `AbstractTemplate.paint()`.
Most uses of this method would fail if they encountered a null value, and very few implementations were able to return null. The few null cases were easily replaced with empty areas instead.
Implementations have access to the zone through `renderer` or `getZone()`, and the passed ID always comes from there as well. In a related change, FOW expose tool implementations no longer look up the global current zone renderer. Instead they just use the renderer that the tool is attached to.
These weren't actually used for anything, but were still being kept up-to-date on vertex and radius changes. With them gone, we no longer need `adjustShape()` or the overrides for `setRadius()` and `setVertex()`.
This renderer was still in use, but it adds some unnecessary constraints to BlastTemplate. Mainly the issue is that it had to modify the renderer's shape, and had to override `draw()`/`drawBackground()` instead of the `paint()` method whichi is more idiomatic for templates. Now we don't even bother keeping a shape up-to-date as we just create a new rectangle whenever it is needed. This is not expensive at all, especially compared to what some other templates have to do.
Similar to BlastTemplate, now we make our own shape on demand.
Now that zones are passed via methods, the templates don't use the zone ID for any real functionality anymore. Removing it cleans up a bunch of code, and avoids the need to make sure that templates have their zone IDs updated in various case.
kwvanderlinde
force-pushed
the
bugfix/4524-4527-disappearing-and-non-scaling-templates
branch
from
34525c5 to
1ca8cc9
Compare
|
Fixed the merge conflicts. |
kwvanderlinde
deleted the
bugfix/4524-4527-disappearing-and-non-scaling-templates
branch
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
Fixes #4524
Fixes #4527
Description of the Change
The main idea with this change is to fix #4527 by passing a
Zoneto templates rather than requiring them to look the zone up themselves. This is done by adding aZoneparameter to theseDrawablemethods, and other related methods:draw()getBounds()getArea()Regular drawings ignore the new parameter, but templates require it in order to scale properly without relying on global state.
There were some remaining zone lookups in
RadiusCellTemplate,BlastTemplate, andBurstTemplate. These all delegated to aShapeDrawableas a renderer, and keeping that renderer in sync with the template state required knowledge of the zone when modifying the template. But these renderer did not add any value to these templates --RadiusCellTemplatedidn't use its renderer, whileBlastTemplateandBurstTemplatewere just as well to draw their own shape. Removing these and the need for maintaining extra state is the resolution to #4524.With those changes done, templates no longer require their zone ID in any way. So that is now removed from
AbstractTemplateand the associated DTOs. This simplifies other parts of the code (e.g.,Zonedeserialization) as we don't have to worry about updating template zone IDs.Possible Drawbacks
Should be none.
Documentation Notes
N/A
Release Notes
This change is