Decompose drawing, expose, and topology tools #5011
Merged
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
refactor/3691-drawing-tool-decomposition
branch
from
c1458ca
to
b8274e5
Compare
Existing tools are all class-based, i.e., are identified by a class, and state is tracked in terms of classes. Now it is possible to define tools by passing instances. The only difference is that instance-based tools cannot be looked up by their class, but that functionality is only used for a select few transitions.
`Path2D` is now transferable in addition to to `Area`, with the latter now being implemented on top fo the former. All `Ellipse2D` are now transferable, whereas previously only `Ellipse2D.Float` was allowed. We always use double precision for ellipses.
The existing `AbstractDrawingTool` is now in a transitional state and will not exist much longer. The new-style tools will not use it, but will inherit from `AbstractDrawingLikeTool`, which provides a bare minimum of functionality. Eventually, `AbstractDrawinTool` will be repurposed into a base class for templates, and even then may not look much like it does now.
This will be useful in conjunction with the new strategies so that we don't have to make assumptions about the result types.
These will form the basis of a new set of tools for drawing, FoW, and topology. Instead of a spawling class hierarchy with complex interdependencies between each type of tool, the new approach will favour these new strategies for defining the shape, while the tool will define what to do with the shape, e.g., add a drawing, expose some fog, or add some topology. By using these new strategies, there will be no more need for FoW and topology tools to inherit from any drawing tools or vice versa.
The new `TopologyTool` uses the previously introduced strategies to define all topology tools. They are instance-based, using the recent supporting change to the toolbox. `TopologyTool` merely interprets the results of the strategies. The strategies are responsible for defining the shapes, but have no knowledge of what the shape will be used for.
Analogous to `TopologyTool`, we now have `ExposeTool` that can work with any strategy and merely interprets the results. This also fixes a minor bug where the iso rectangle tool did not result in the fog border being used.
Analogous to `TopologyTool` and `ExposeTool`, we now have `DrawingTool` that can work with any strategy and merely interprets the results as drawings to add to the zone.
`Oval`, `Rectangle`, and `Cross` no longer serve a purpose and so have been deprecated. It is possible for existing campaigns to have these serialized in them, so we are keeping the structure around. However, each one will `readResolve()` itself into a modern type. Also be clear about the set of supported types in `ShapeDrawable`. The new `ShapeDrawable#getShapeTypeName()` outputs one of `"Rectangle"`, `"Oval"`, `"Polygon"`, `"Area"`, or `"Unknown"`, which callers can use to make meaningful decisions about the general type of shape wrapped in the drawable. This matters especially for ellipses, where several places refers to the simple name of `Ellipse2D.Float`, which is just `"Float"`. Now they refer to `"Oval"`, or use proper type checks where needed.
kwvanderlinde
force-pushed
the
refactor/3691-drawing-tool-decomposition
branch
from
b8274e5
to
b55ccfb
Compare
cwisniew
approved these changes
Nov 5, 2024
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
Supports #3691
Partially addresses #5004 (just for drawings)
Fixes #4218
Description of the Change
This PR replaces the deep
AbstractDrawingToolhierarchy strategy-based classes. The goal here is to consolidate each tool's logic so it takes fewer lines of code and isn't spread across several classes, while still reusing code. Future work for #3691 will build off this now that it is easier to change the rendering logic of each tool.The drawing tools, FoW expose tools, and topology tools have all been refactored. There are now only three tool classes for all of them:
DrawingTool, for any drawing toolExposeTool, for any FoW expose toolTopologyToolfor any topology toolThe different shape are obtained by strategies that can be plugged into the above tools:
RectangleStrategyfor drawing any type of rectangleOvalStrategyfor drawing any type of ellipseIsoRectangleStrategyfor drawing any type of isometric rectanglePolyLineStrategyfor drawing any type of polylines, polygons, and freehand lines.CrossStrategyfor drawing any type of X (only used for topology right now)These strategies produce idealized shapes, which in practice are
RectangleandPath2D. They can be used for either hollow or filled shapes, which is why there is only one of each. It is up to the tool itself to decide whether to fill the shape, whether it needs a stroke, and what the final shape means. The strategies can also optionally provide measurements of their shape, which is how the drawing and expose tools now show those.These new tools provide the user with more of a WYSIWYG experience: the temporary shape drawn by the tool will exactly match the final outcome. This effects a lot of the tools, but is especially noticable for the ellipse topology tool - it results in a decagon, and the tool now shows the user the exact decagon they will end up with, rather than showing them a larger idealized ellipse.
The implementation of the template tools have not been touched as they are quite different and would be a lot of extra effort. I plan to do something about them in a separate PR.
Supporting changes
Toolbox
The toolbox used to only work with tool classes, which forces the need for the deep tool hierarchies. In order to make it work for the new strategized tools, the toolbox was extended to allow adding
Toolinstances. The behaviour is much the same, except that it is not possible to look up an instance-based tool by itsClass<?>.Legacy drawings
These
Drawableimplementations were only used by the tools and therefore are no longer used:-CrossOvalRectangleThey have all been marked
@Deprecatedto discourage any further use. Because they might have somehow been serialized in existing campaigns, they have been kept around, but reduced to just their fields and no behaviour. They all usereadResolve()to replace themselves with aShapeDrawableinstead.Protobuf
The set of shapes representable in protobuf was expanded slightly:
Path2Dcan now be sent in addition toArea. This was originally meant to be used in this change, though in the end it was not necessary. But I decided it was still worth keeping around sincePath2DandAreashare the same logic.Ellipse2Dand be sent, not justEllipse2D.Float. On the other end, it will be mapped back to aEllipse2D.Double.The
Ellipse2Dhad some knock-on effects. The trivial one is that we now useEllipse2D.Doubleeverywhere and never useEllipse2D..Floatanymore. Another issue is that we actually rely on our ellipses being backed by a class with the simple name"Float", as it is for inEllipse2D.Float. Some of these examples were doing type checks this way and have been updated to useinstanceof Ellipse2Dinstead. Other places genuinely needed a string from aShapeDrawable, so there is now aShapeDrawable.getShapeTypeName()that will return a name for any support type, with ellipses being given the name"Oval".Possible Drawbacks
This is a big change, so there's a chance I missed something in testing that doesn't work quite how it used to.
Because of the slight differences in drawing, expose, and topolgy tool behaviour (see #5002), there is some logic duplication between these tools. It's more of a mess to try avoid this. If we ever implement #5002, the tools can be simplified quite a bit further.
Documentation Notes
N/A
Release Notes
This change is