beardgame / td

Multiplayer Tower Defence game. Systemic component game design. In-depth elemental tech tree. Environmental storytelling. Explosions.
9 stars 1 forks source link

🚧 Make UI Builders non-static #395

Closed tomrijnbeek closed 5 months ago

tomrijnbeek commented 5 months ago

✨ What's this?

This PR is a first pass over all the UI builder & factory code to make them no longer static, but rather have state so that they can have internal references to animations, tooltips, etc.

🔍 Why do we want this?

Right now we have to pass in things like animations and tooltip factories into builders if we want the builder to make use of them. In the current state, that means animations are not consistently used for buttons, and adding tooltips to buttons is a pain, even though we should really be doing that wherever we can.

🏗 How is it done?

I decided to start with the button factory, since it's so important. It turned out I was right, since basically everything had to be migrated based on that. In the end, we have the following hierarchy:

🦋 Side effects

🤔 Open for debate

The following points are still discussable, as I am not 100% sure on these decisions.

paulcscharf commented 5 months ago

UIFactoryContext? UIBuilderContext? UIControlContext? UI(Control)HierarchyContext? xD

I'd vote for at least passing the context into all controls, to just keep that unified and simple. I'd also be happy to pass it into all the builders as well, instead of only parts of it. We might need animations, sounds, and in the future particles and other things in more and more places, I don't see any benefit of not passing in the context, making ourselves more work down the line, and actually ending up in more code generally.

That being said, I don't like that context.Factories.Button() is very verbose. I already feel that factories.Button() is slightly unfortunate. Though perhaps it helps if we rename it to Builders, or simply Build or Make?

context.Builders.Button() 'context.Build.Button() 'context.Make.Button()

Make might be my favourite one, being a verb (compensating for Button() etc. not being one).

It doesn't make it much better, but... yeah, not sure. I don't think the extension methods should be on the context directly though, since that has more things in it. So maybe it is what it is.

I almost wish there was such a thing as extension local methods so you could easily share something like:

ctor FancyControl(UIContext context)
{

var button = Button(...);

Button Button(Action setup) => context.Factories.Button(setup)
}

But I'm probably getting too clever xD

Anyways, except for just passing in context, and maybe some of this naming stuff - feel free to merge at your convenience!

paulcscharf commented 5 months ago

Final thought: We could make a base class:

abstract class CustomControl(UIContext context)
{
    public UIContext Context { get; } = context;
}

And make the builder extensions be extension methods on this class...

Except you still have to call this. for all of them then. Not sure if much is gained really. If you just want to go with KISS on this, feel free.

tomrijnbeek commented 5 months ago

UIFactoryContext? UIBuilderContext? UIControlContext? UI(Control)HierarchyContext? xD

It doesn't get better does it?

I'd vote for at least passing the context into all controls, to just keep that unified and simple.

Fair enough, I'll update

I'd also be happy to pass it into all the builders as well, instead of only parts of it.

How will you pass in the context in the factories if the factories are in the context? 😁

Maybe some builders will even require dependencies that aren't in the context in the future.

That being said, I don't like that context.Factories.Button() is very verbose. I already feel that factories.Button() is slightly unfortunate. Though perhaps it helps if we rename it to Builders, or simply Build or Make?

context.Builders.Button() 'context.Build.Button()'context.Make.Button()

Make might be my favourite one, being a verb (compensating for Button() etc. not being one).

It doesn't make it much better, but... yeah, not sure. I don't think the extension methods should be on the context directly though, since that has more things in it. So maybe it is what it is.

I almost wish there was such a thing as extension local methods so you could easily share something like:

ctor FancyControl(UIContext context)
{

var button = Button(...);

Button Button(Action setup) => context.Factories.Button(setup)
}

But I'm probably getting too clever xD

I hear you, but I think any method to make this shorter simultaneously makes it less obvious/clear.

Seeing where the Button method comes from to begin with (even though it's an extension method, so it's not quite coming from the factories, but it's a clear indication it's the major dependent type) is actually a benefit in the code comprehensibility in my books. This is also why Factories is the right name in my opinion, because the ButtonFactory really is a factory (based on the factory pattern). If you want a Make, you can make it an extension method that returns the factories or an empty object with a reference back to the context/factories on which you can add the extension methods. But it doesn't feel necessary.