[Refactoring]: Split up ZoneRenderer to enable an easier transition to other rendering implementation

[kwvanderlinde]

Describe the problem

The ZoneRenderer class is both large and too interconnected with itself. Making changes to it is always risky as it's not clear which parts of the ZoneRenderer will be affected and which will not. On top of that, large and fundamental changes such as moving to different renderers (e.g., libGDX) are particularly difficult, at least not without duplicating core logic and bringing associated clutter.

The improvement you'd like to see

I'd like to see the ZoneRenderer broken down into parts that can work together but are much more managable.

1. The rendering logic under ZoneRenderer#paintComponent()/ZoneRenderer::renderZone() is split up into separate render layers.
   • Each layers would maintain its own internal state (e.g., caching light sources in the lighting layer) so that the ZoneRenderer has less state.
   • The ZoneRenderer would have much less logic in it, and the rendering operation would be simplified to drawing each render layer in turn.
   • This would make rendering changes more focused as modifying one render layer is not as likely to interfere with another.
2. ZoneRenderers nature as a Swing component is removed. Instead, the ZoneRenderer can expose a reference to some Swing component, but which component is determined by something other than the ZoneRenderer.
   • Another rendering implementation (like libGDX) could then provide a different Swing component than the current one.
   • The ZoneRenderer would only be responsible for providing an interface to the visible zone, while the Swing components would be responsible for effecting renders.
3. Define a graphics context that provides the necessary fundamental operations we need to draw a zone.
   • E.g., operations for drawing an image at a certain location, for transforming the view port, and for setting clips.
   • We can make more than one implementation. E.g., a Swing implementation that interfaces with a Graphics2D, while a libGDX implementation could do whatever it needed to do.
   • The render layers would use this graphics context instead of relying directly on either a Graphics2D or any libGDX stuff.
4. Pull rendering logic out of non-renderer components.
   • E.g., remove Grid#draw() and Grid#drawCoordinatesOverlay() and instead make that the responsibility of a render layer.

The end result (if all my ducks are in a row) is that:

• The ZoneRenderer defines some core logic, such as what order to render things in, what actually needs to be rendered, etc.
• The logic behind rendering is determined by the various render layers. E.g., deciding where to paint light sources in what shape, etc.
• The details of how to render each element is determined by our graphics context.
• The model and other parts of the code are free of any dependency on swing when rendering in the zone.

Expected Benefits

The main benefit, regardless of a move to libGDX or anything else, is that the ZoneRenderer will be more manageable. This is mainly offered by point (1) above, but the other points could also help with that.

The secondary benefit is that it would be relatively easy to add a libGDX renderer:

• No need to duplicate core logic in ZoneRenderer.
• Only need to provide a Swing component that can be integrated into MapTool, and an associated graphics context to handle drawing operations.
• If rendering layers needed to be changed to support libGDX, those changes would be more obvious and self-contained.

Additional Context

This refactoring suggestion is motivated by the hope of achieving a libGDX renderer as in #3468.

[thelsing]

Currently my GdxRenderer lives on Top of the ZoneRenderer. It delegates Stuff to the ZoneRender. ZoneRenderer skips drawing when GdxRenderer is visible.

I also would like to see that split. But hopefully after the experimental renderer is merged ;) My gdx-Renderer is currently stuctured like the ZoneRenderer. I would rather not have to rewrite it completely. It nearly works now. (Lights have problems still). If you like feel free to pull my feature-gdx branch. Any help is very much welcomed.

I think the renderers should only render the zone. Macros and everything should work even if you use a DummyRenderer that does nothing. I would also like to have only one instance of the "ZoneManager" oder "ZoneEngine" class if possible. Not sure if macros need it for every possible Zone.

I also would like to see the drawing code of various Grids and Drawables separated.

[thelsing]

For 3 I think an interface to notify the renderer that it should rerender + changes for scale etc should suffice IMO. We should use the Renderer of a kind of facade. This way different renderers can have a different complexity. Every Renderer should manage the its context by itself.

[kwvanderlinde]

Sorry I kind of left this, life got in the way and then I didn't have time for such a big change like this.

I also would like to see that split. But hopefully after the experimental renderer is merged

I would say before the merge, or as part of it. There's a lot of duplicated logic between the two, and (as someone who touches vision now and then) I don't want to have to make sure the two stay in sync.

I think the renderers should only render the zone. Macros and everything should work even if you use a DummyRenderer that does nothing. I would also like to have only one instance of the "ZoneManager" oder "ZoneEngine" class if possible. Not sure if macros need it for every possible Zone.

This is very well put. I agree that there should be a separate decision-making component that decides things like token visibility, etc., and that should be independent of the renderer itself. Focusing on that will help a lot while being way simpler than my layered proposal (that can be done in the future if at all).

I'm going to try put a change together that splits the current ZoneRenderer into a "ZoneManager" (or whatever we call it) from a SwingRenderer. The changes are going to be made with GdxRenderer in mind so that the functionality there can mostly be preserved, just cleaned up. The pain point is going to be pulling out some of the important update logic from the rendering logic, especially in same big offenders like renderTokens().

[bubblobill]

I'm going to have a go at this at a very basic level. This is the plan. Move ZoneRenderer to its own package net.rptools.maptool.client.ui.zone.renderer Work through the code:

1. identify logic/decision tree
2. split out the decision tree actions to methods so the core code is - "If this do that"
3. find methods worth separating into classes
4. hopefully identify repetitious code worthy of separate methods or splitting off into a utility class

[kwvanderlinde]

Seems some graphics settings are not being consistently applied to the notes at the top (e.g., "Map not visible to players").

On a new empty campaign: On certain campaigns / maps: I don't know what the common thread is yet, some maps have the problem and some don't

Bisected to 4938b03c4

[kwvanderlinde]

Seems some graphics settings are not being consistently applied to the notes at the top (e.g., "Map not visible to players").

On a new empty campaign: On certain campaigns / maps: I don't know what the common thread is yet, some maps have the problem and some don't

Bisected to 4938b03

Narrowed that down to whether a label is present on the map. A blank campaign is fine, but add one label and the rendering gets messy.

I think this is a bug with some caching in Graphics2D - e.g., comment out the g.drawString() calls under ZoneRenderer.renderLabels() and the problem goes away. Though I don't see what 4938b03 has to do with it. Regardless, we should be a bit more hygienic with our Graphics2D objects and that would also fix the problem.

[kwvanderlinde]

Having a look at pulling out renderPath(), and if I get more motivated also pulling out showBlockedMoves().

[bubblobill]

https://discord.com/channels/296230822262865920/494598206626463755/1184546002728914954

If we generate the path area like this it can be used elsewhere.