evaera / matter

A modern ECS library for Roblox.
https://eryn.io/matter/
MIT License
144 stars 34 forks source link

Add loop parameter names #65

Open jackTabsCode opened 10 months ago

jackTabsCode commented 10 months ago

Adds an extra field to the Debugger called loopParameterNames to specify the names you want displayed in the Matter debugger, rather than the default World 1, table 2, table 3, etc.

Ukendio commented 8 months ago

I see what the intent with the PR is but I am not a fan of the execution.

I think we should ask the others that are working on the debugger cc @LastTalon @metrowaii

Edit: To elaborate, I think it is not very ergonomic for the user to have a separate field that takes a table of strings. Edit2: Read comment below

Ukendio commented 8 months ago

I think we want to make the debugger more modular and maybe later down the line we can address this. I think we should close this and maybe revive it until then.

jackTabsCode commented 8 months ago

You're welcome to suggest a different implementation, but the debugger being pulled out into a module or not wouldn't really have an effect on this PR.

LastTalon commented 8 months ago

We can potentially revisit this interface later if we do more debugger reworking. For now, however, this is the interface that I think is the least intrusive and still has the desired effect.

The nearest alternative that has the least intrusion is to set the __tostring of the parameters, but this isn't an ideal scenario as, for instance, World objects shouldn't have people reaching into them to adjust the __tostring. And this is the case for many such objects that users may pass to the loop.

And in reality, this is not meant to display what the stringified object is, but the label we choose to give it. E.g. if we pass an actual string as a loop parameter, we don't really want the debugger to display the string in this place, we want a label for what the user calls that parameter. So these are actually different things. Thus, as far as I can tell we must provide an additional string here because the semantics of that string are for what the user wants to label their parameters. And it has to be given to the debugger because the loop doesn't care what the user calls them, only the debugger does.

Feel free to leave additional comments. Jack and I worked on the interface for this together and this is what we settled on as a minimal implementation that hits all of those points.

Ukendio commented 8 months ago

I am unsure of a better implementation but keeping a separate table of strings makes it more onerous as to which label corresponds to which object imo. I understand the problem that this has to be done at the debugger. Which is why i propose an orthogonal api that would be something like this

local debugger = Debugger.new(Loop):add(world, "World"):add(state, "Table 1"):create() local loop = debugger:getLoop()

After the arguments have been added and the debugger is built, we add the arguments in order to create the loop class.

I think this makes the expression more intentful with the labels. However the drawback is very clearly that to get the loop instance you have to get it from the debugger which feels like it has a pretty backwards relation. Not sure if there is a better way to do this that keeps a flat topology.

LastTalon commented 8 months ago

I don't think this is a good choice of API. The debugger shouldn't be creating a loop. If you think the parameters need to be associated, I understand that argument and in that case we should pass a map. But I don't really think I agree, again, because of the way the parameters are given, not by value, but by position.

If we look at proposed API in usage this is very clear and even symmetrical.

local loop = Loop.new(param1, param2, param3)
debugger.loopParameterNames = { "World", "State", "Widgets" }