Kruptein / PlanarAlly

A companion tool for when you travel into the planes.
https://www.planarally.io/
MIT License
393 stars 70 forks source link

Set note icons into their own category of shape for rendering #1409

Closed rexy712 closed 4 months ago

rexy712 commented 4 months ago

Note icons are currently drawn as an ordinary shape which works in most ways. Since they are lumped in with all other shapes in a given layer, there exist some problems such as #1406 when it comes to rendering priority. The shape could become obscured by the shape it is supposed to be attached to.

This PR introduces a new class of shapes in a layer called dependent shapes. These are treated as shapes that depend on another 'parent' shape to exist and are therefore always drawn on top of the parent shape but still remain underneath other overlapping shapes as one would expect. This new class can be utilized to allow drawing of other icons such as an invisibility indicator in the future.

This solves #1406.

Kruptein commented 4 months ago

I did a quick glance through the code and you seem to have thought of plenty of cases, which is nice.

Is there a particular reason why you chose to track the dependents on layer level instead of on the shapes themselves? Because I feel like some logic would not be necessary in that case (e.g. layer changes). It's not a dealbreaker for me, but something I was curious about.

I have a busy day, but I'll try to give this a proper review tomorrow.

rexy712 commented 4 months ago

I had just thought it fit better with how the data was stored for everything else. It does make it easier to put the dependents in the shape itself. I can do that without too much work from what I have now and simplify the logic in some places.

I'll wait for your review before changing it.