Add a basic Overlay class and OverlayManager

[kristofbolyai]

This PR is currently WIP, I created this so the team can see and discuss if they like this system.

TODOs (not fully in the scope of this PR, but I may include them):

• Overlay: This class is very bare bones at the moment, it should be improved. (This PR is focusing more on the OverlayManager and render system, the Overlay class should be detailed more in a separate PR)
• RenderEvent: Adding more ElementTypes and canceling will allow us to "override" GUI parts like the health bar, necessary for some overlays.

Here is the render of the example overlay I added, called DummyOverlayFeature:

[kristofbolyai]

I would like your opinion on the direction of this system, whether this is going in the right direction, or you are imagining something completely different. If you think this is a nice base, I will continue working on the TODOs listed above.

[DalwynWasTaken]

I think this is heading in the direction of having them both separated from features but allows features to own overlays. Which is good, I do believe the TODO's should be implemented at the basic level before any clear judgement can be given.

And while you work on it, having overlay have some sort of overlayconfig field which would be the thing stored cross instance (location and if it's enabled. Everything as seen by the settings) or it could be OverlayPosition and you'd add config annotation if you want it saved. But this is more of a "keep this in mind" thing

[P0keDev]

I like the setup of inner classes for overlays, but I don't think doing everything via annotations/reflection is the cleanest way to accomplish this, especially with the idea in mind of registering multiple instances of the same overlay. My immediate thought is that overlays should be explicitly registered in a feature's init method, which will also attach positional data (for configs) and maybe render data, although the latter could be kept as an annotation on the inner class. These overlays will be registered in a list within the feature, much like keybinds, so that when the feature is enabled or disabled, the feature can register/unregister all of its overlays with the OverlayManager. Here's a rough example:

public class SomeFeature extends UserFeature {
    @Config(...)
    OverlayPosition position1 = new OverlayPosition(...);

    @Config(...)
    OverlayPosition position2 = new OverlayPosition(...);

    public void init() {
        registerOverlay(new SomeOverlay(), position1);
        registerOverlay(new SomeOverlay(), position2);
    }


    @Overlay(renderType = HUD) // etc
    public static class SomeOverlay extends Overlay {
        public void render(...) { ... }
    }
}

And then in Feature:

    Map<Overlay, OverlayPosition> overlays = new HashMap<>();
    // ...
    protected void registerOverlay(Overlay overlay, OverlayPosition position) {
        overlays.put(overlay, position);
    }
    // ...
    public final void enable() {
        // ...
        OverlayManager.registerAll(overlays);
        // ...
    }

[Incompleteusern]

Looking good, but I have a few complaints. I feel that OverlayManager should retain information about the feature that added that overlay. On that note, I think registeredOverlays in OverlayManager is unnecessary, and if a check that the registered overlay is a child class of the feature, it can be checked on initialization. If for some reason a feature needs to instantiate multiple overlays of the same kind, maybe use ids. Lastly, I feel that overlays should be created only when the parent feature is enabled/disabled.

[kristofbolyai]

@Incompleteusern @P0keDev I implemented your suggestions, but the two of you had a little conflict of interest, so let me know if you don't like something.

[P0keDev]

Personally I would prefer it if overlay registration paralleled keybinds in its design (features keep track of their own overlays, register/unregister them to the central handler as needed). OverlayManager could keep track of overlays' "parent" features in addition to this, if that's also important. All up for debate, however

I would also advocate for renaming the overlay stuff to parallel the names of our feature framework, i.e. Overlay -> OverlayInfo and OverlayBase -> Overlay

[magicus]

Overall, I think this is definitely heading in the right direction. I'll add some specific remarks in the code shortly, but let me first discuss one overall thing.

I think the code is getting unnecessary complex due to the idea that enabling overlays mean instantiating them. Instead, I think we should treat overlays like features. Always instantiate them in the beginning. To enable/disable an overlay really just means to move it on or off the list of enabled overlays, which is traversed when calling the render methods on overlays. There is no cost associated with having an instance of an overlay that is created but not being called, so we should not be afraid of this. So enabling/disabling really just means shuffling instances from enabledOverlays to disabledOverlays or back again.

Second, while I originally championed the idea that we should allow a feature to have multiple instances of an overlay, I also must stress that this is a special case scenario (only info boxes seem to be relevant atm), and it's better if that part gets a bit messier, if it can make the normal case of one overlay instance per Overlay class simpler. I'm thinking something like this. Instead of registering OverlayPositions as @Config, lets do something like this:

public class SingleOverlayFeature extends UserFeature {
  @Overlay(name = "Fancy overlay", renderType = RenderEvent.ElementType.GUI)
  private Overlay fancyOverlay = new SingleOverlayFeature.FancyOverlay();

  public static class FancyOverlay extends Overlay {
      // ...
  }
}

public class MultiOverlayFeature extends UserFeature {
  @Overlay(name = "Tribbles overlay", renderType = RenderEvent.ElementType.GUI)
  private List<Overlay> tribbleOverlays = new ArrayList<SingleOverlayFeature.TribblesOverlay>();

  // ...
}

That is, the @Overlay annotation supports either an Overlay class directly, or a List of Overlays. In the latter case, we probably need to notify the OverlayManager whenever the list is updated. But, once again, since this is a special case, I think we can live with that. It's just two simple calls, whenever we add or remove overlays on the list.

[magicus]

I would also advocate for renaming the overlay stuff to parallel the names of our feature framework, i.e. Overlay -> OverlayInfo and OverlayBase -> Overlay

I second that. It really fits in better with what we have, and feels like more natural descriptions.

[kristofbolyai]

Third iteration! Tried to do everything as requested, although @magicus, you did not mention how you would store OverlayPosition, so I bundled it into the Overlay class. Overall the code is now simpler now. As requested by @P0keDev, the Feature class now holds information about its own overlays.

[magicus]

I think this is starting to look pretty sweet! It feels like we're almost there, at the Perfect Overlay Design. :-)

@kristofbolyai I think bundling the position in the Overlay class was exactly the right decision. I just have three issues surrounding the overlay position thing.

1. We need to get back Dalwyns idea of storing the position as a configuration item into this framework. I'm not sure if we can mis-use the @Config annotation on the Position field in the Overlay, or (more likely) if we need to do some custom adaptation on configurations to get this to work smoothly. (We need some nice name for the configs, like <feature>.<overlay>.[x|y], and I don't think we can generate that currently).

2. How should we do the initial, default position? It's one thing that if the user has dragged around the overlay box that we persist their choice. But it seems sub-optimal to hard-code the initial position as a pair of integers in the constructor of each Overlay.

So even though I think an Overlay should hold it's position, we could perhaps allow it to be null, and not set from the constructor. And then maybe the OverlayManager can have some way of assigning default positions for the overlays? After all, it's a bit of a "holistic" system wide thing, to get a proper default where different overlays have good default that work with each other.

3. Right now the assumtion is that an Overlay has a fixed width and height. I think we will need to challenge this assumption rather soon, to allow resizable Overlays. We might consider preparing for this by storing the size not as a width and height field, but as an OverlaySize, similar to the OverlayPosition. An OverlaySize class can be subclassed with a FixedOverlaySize, or an RestrictedRangeOverlaySize, etc, to cater for different needs from different overlays. And if the OverlaySize has user changable values, they too need to be persisted in the config file.

But we can also accept this PR as it is now, and continue working on these aspects as follow up issues.

[kristofbolyai]

I would separate Overlay config saving into a PR, and then advanced Overlay characteristics into another one.

[magicus]

@kristofbolyai I'm perfectly fine with that. Sometimes it's easier to have a checkpoint with a PR committed, and then you continue to refine details separately. Feel free to postpone any of my comments to a later PR, as long as they are adressed in some way or another later on. :-)

[magicus]

I thought DebugFeatures did just that..?

[Incompleteusern]

DebugFeatures should only enabled for dev environment, this should be never be

[kristofbolyai]

We could move this into an examples folder and not register it?

[magicus]

Whatever, it does not matter so much. I imagine we can remove this thing as soon as we start getting real examples of the Overlay system. I'm fine with a commented out line like this too.

[magicus]

I just discovered that we have an @StartDisabled annotation for Features. I assume this does what you wanted? :-D

[magicus]

This looks soooo epic! 🥳