Formspec Refactor

[rubenwardy]

This thread is for technical discussion about the code behind formspecs. It's not about the formspec language itself, but the implementation of it. Please do not request any user-facing features here

Formspecs and GUIs have been improving significantly lately. Now, all physical formspec elements create IGUIElements.

Discussion: http://irc.minetest.net/minetest-dev/2020-02-02

Aims

• Separate Formspec parsing and formspec-knowing code from the rest of the GUI code. This will allow us to change the serialisation format in the future
• Integrate most logic into IGUIElements rather than in the guiFormspecGUI class, so that it can be shared
• To be discussed

[ghost]

I think it might be useful to either start a document somewhere with the high-level goals of formspecs (ie, what is our end-goal for this API?) that people can work on, or have some sort of 'meeting' in the dev IRC.

At this rate, I get the impression that this discussion might last months without going anywhere otherwise.

Of course, this is really just more talk... I'm going to try drafting a larger proposal sometime this weekend, and I'll post it here for discussion/feedback.

[ghost]

Alright, so I was busy last weekend and this week but here's a general idea of what I'd like to see happen:

Initial Proposal

This is a proposal for refactoring formspecs and nothing else. It's not about adding new features, but instead primarily aimed at housekeeping and making things easier to extend, test, and maintain. With that said, it's worth restating some of our long-term goals and keeping them in mind when designing the new architecture to avoid future roadblocks.

Longterm Goals

• Some form of dynamism (data bindings, partial updates, UI tweening, etc)
• Removal of Irrlicht (I don't think this is controversial? Either way, being less dependent on one API is generally good)
• Automatic layouts (Grid layouts, dock layouts, lists, etc)
• Support future API/Serialization changes

Current major issues

• One god class, GUIFormSpecMenu, does basically everything and is becoming harder and harder to maintain
• Heavily dependent of irrlicht's API, while benefiting less and less from it (which results in a lot of stuff getting thrown over it, like styling for instance)

Proposal

Management

• Replace global MainMenuManager with a constructor-injected object that exposes a simple API for pushing/popping GUIs
  • We only use the current manager in a handful of places, so it should be fine
• When creating new GUIs, we don't pass the manager into them. Instead, we pass them into the manager. GUIs shouldn't know who's 'managing' them
  • The manager can listen for when the GUIs want to close, instead of the GUIs removing themselves

Parsing

• Register parsers by element name in a constructor-injected registry object (Good for testing)
  • Parsers provide a method for creating elements, plus some extra information (this creates a scope, this destroys a scope, etc)
• When receiving a formspec, run it through a parser object that looks up element parsers in the provided registry
  • Maintain a 'scope' stack for building the tree and propagating styles
  • The result of the parser can be passed into the menu manager by the parser's owner

Structure

• Formspec becomes a tree of elements
• Elements have a list of children, update/draw/event handler, geometry, flags
• Adopt a GTK-esque loop structure:
  • Process events and 'tick' applicable elements (animated image, for instance)
    • Events should start as 'deep' in the tree as possible. Each element receives the event and returns a 'handled' value. Until handled is true, the event propagates towards the root.
  • Perform layout on 'dirty' elements
    • Right now, elements will start dirty and layout will simply move them to where they need to be. This will allow us to add automatic layouts, resizing, etc later though.
  • Render elements

So how do we do this?

• Start by replacing the manager and menu classes to fix the access/control issues
  • ...and unit test them
• Create the parser/registry and clean the parsing out of GUIFormSpecMenu
  • ...and unit test them
  • We can temporarily copy all the parsing mess into this class, and use that mess as a fallback when the registry comes up empty. This lets us use the registry for new elements, and gradually convert old ones.
• Make a proper class for all UI elements.
  • At the same time, get all classes to derive from a simple wrapper that hooks up our new API to the Irrlicht API
  • We should consider any non-wrapped Irrlicht types in new classes a blocker for merge. Let's not make our problems bigger than they are today.
  • Each time we do this, we should also refactor the element's parsing to be registered instead of directly baked into the parser
• Once this is done, we can remove all remaining Irrlicht references from the gui part of the codebase. Ideally, this will put us in a good position to continue improving formspecs and simultaneously lay the groundwork to let us limit the 'Irrlicht footprint' of other systems as well.

Since this proposal was slightly rushed, I'm sure I've missed some things here. Still, I think this offers a much more concrete plan and an actual roadmap for executing it! I welcome feedback, let's try to make this a good refactor.

[Desour]

Actually, theoretically the irrlicht gui system isn't that bad. It's just not easily extensible enough.
Eg. the IGUIEnvironment has some IGUIElementFactory that seems to read elements from strings and lets you register new ones.
Completely reinventing the wheel would be a bit absurd.
But as the irrlicht gui system is old and a pita, we should maybe learn how it can be used and improve it after adding the irrlicht gui source code to minetest.
Maybe another option would be to include some other gui system (like qt or gtk) to minetest and overwrite the drawing, so that the look doesn't change.
TL;DR: 3 Options: reinvent the wheel (+freedom -work -the outcome might be bad), override the irrlicht sourcecode (+less changes -eww, irrlicht) or maybe include a new gui library (+such systems are up-to-date and probably next to perfect -I'm not sure if it's possible)

Warning, the following section is about the problems of the limitations of the current formspec language and hence a bit offtopic:
A big problem of the current formspec system is that it uses a non-hierarchic format. Eg. we have an image-button element type because if you would place the image in front of the button, you couldn't press the button, having other elements (like read-only-textareas) above clickable things (like buttons) is still impossible.
As GUIs are normally organized hierarchically, I suggest to switch to a hierarchic format like xml (of course the current format should still be supported). This would probably also allow leaving parameters empty and make it easier to read the formspec strings (eg. there are always some weird numbers where one can't see on the first sight what they are used for, like button[3.12,4.01;2.53,0.76;btn;bla]).

[rubenwardy]

Pulling the irrlicht GUI source code into our own code and refactoring it is likely to be a good bet

[ghost]

The Irrlicht gui system isn't bad, but it's not meshing well with what we're doing and the engine is more or less on life support, leaning on it as we have is not really a good idea. I'm also a little worried about carrying on Irrlicht... it's pretty old tech at this point, and I get the impression that we might be stuck redesigning it later to get new features/graphics APIs working someday. This is why I support wrapping and abstracting as much of it as possible and building a new system on top, it lets us change tech later without eating a massive cost.

Maybe another option would be to include some other gui system (like qt or gtk) to minetest and overwrite the drawing, so that the look doesn't change.

Definitely wouldn't support either of these options, integrating them into another renderer would be a nightmare and they're not designed for games at all. Maybe something light like imGUI or nuklear, but the problem with these is that they're designed more for immediate-mode UIs, not really the kind of big structured over-the-network UIs that formspecs present.

I agree with your second point, but it's out of scope for a refactor like this... regardless, you might've noticed that I support using a proper hierarchy in the backend so that we can transition to such a format in the future.

[ghost]

I guess, honestly, the thing I'm worried about is that it feels like we have already reinvented the wheel, and formspecs have some pretty particular requirements that most conventional UI engines aren't designed for (defining/modifying a UI on one end of a network and presenting it on another, only the web follows this pattern).

To me, that implies that as long as we try to rely on conventional solutions, we're going to have to hack them to do what we need.

[rubenwardy]

imGUI

This is not appropriate for production games - it says this at the top of the readme

[v-rob]

I don't think ditching Irrlicht is the way to go. Instead, we should keep Irrlicht, but have an abstraction layer. The current way we handle elements is that they have a FieldSpec associated with them, so to modify them, we need to use getElementFromId and then static_cast it to the appropriate element using the type from the FieldSpec. Yuk. We need something that handles the elements, not GUIFormSpecMenu.

As for XML, I don't think that's a very good idea. This is used by the web, but the web wasn't designed with interactivity in mind at all. JavaScript came from Netscape Navigator. If you look at most traditional GUI libraries, they use functions to create elements with the resulting variable referencing that element. Very similar in design to Minetest's HUD. I think that this will probably end better than building FormSpecs on XML. But, as said, this is offtopic because we are just working on the internal code for now.

[rubenwardy]

@Unarelith I know you've left, but any thoughts on this?

[Unarelith]

See http://irc.minetest.net/minetest-dev/2020-03-02#i_5646956

[rubenwardy]

Based on the comments above, I've made a design focusing on the parsing side of things.

Source UMLet file: formspecrefactor.uxf.txt

Some notes:

• Formspec parser should have the formspec element parsers dependency-injected in
• GUIFormspecMenu should have the formspec parser dependency-injected in
• Rather than each element parser handling the formspec format, a FormspecElement is used to represent a generic formspec element specification

Remaining questions:

• How to handle field submissions well? We don't want to corrupt our GUIElements with formspec-related serialisation
• How to handle inventory list selections and actions?
• Future GUIElements architecture
• Testing, dependency-injected

[rubenwardy]

Feedback please

[ghost]

Let me be clear--on my end, my lack of feedback is a lack of objections. I generally agree with your proposal, and have since you made it.

Unfortunately I don't have much to add for your questions, since I haven't touched the current field submission/inventory code.

[v-rob]

I'd really love to give good feedback, but I'm no computer science person. I self-taught myself all my programming knowledge, and I really don't know what constitutes a good organization for a large project. That being said, I'll try my best, but take it with a grain of salt.

• Where exactly will StyleSpec fit into all of this? StyleSpec already has some of its own parsing functions which are used by not only GUIFormSpecMenu, but also GUIButton and probably more elements to come. I feel that StyleSpec's parsing code should come from FormSpecParser and company.
• How will named arguments fit into all of this as shown in your proposal in Formspec replacement #6527 (comment)? It seems that it is numbered internally -- will the named arguments get converted to a number? Where, since FormSpecElement isn't a place for specific element parsing, which named arguments requires?
• Where will FieldData get its values for each specific element since each has it's own different return value for an event? I'm guessing FieldParser has something to do with it, but it's not very obvious what that does or how it's accessed.

[rubenwardy]

1. Stylespec is a key value store independent from formspecs. The parsing isn't exposed to the user of a stylespec, I'm not too concerned about passing it around in terms of later deciding it is formspec specific
2. My formspec replacement proposal doesn't apply to formspecs. The Lua DSL will not compile to the formspec language. Formspecs already have named arguments - I call them properties here. The style element is an example of properties
3. I have no idea, this is one of the questions in my post - question 1

[v-rob]

I believe I've come up with something essential to refactoring formspecs after some failed work on a PR I was working on. Please read and give commentary; I think I've stumbled upon one of the largest problems with formspec code and why it is so non-extensible.

I think giving ParserState (formerly parserData) everything is a HUGE mistake. Let me explain by example: In a web browser, if you give a CSS value like calc(15px + 25%), it won't calculate that into pixels directly in the parser, but in the layouter. If calculated at the parser level, resizing the window or changing something with JavaScript would require a complete reparse, which is what formspecs do currently. Instead, these need to be properties of the layouter.

Specifically, ParserState should only have things related to parsing. A good check is this: If the screen is resized, will the parser ever need to be called again? Parsing should include splitting the formspec string up, getting individual parameters/properties, checking validity of syntax, and then constructing/changing an element. So yes, real_coordinates, explicit_size, current_parent, and scrollbar_options are all examples of things ParserState needs. What does not belong are other things, especially layouting, e.g. imgsize, size, anchor, screensize, etc. The parser must do its job at parsing, nothing else. Leave the other stuff to other classes, e.g. a FormspecLayouter class. Each element needs its own parser function, but it also needs its own layouter function.

I think this idea of parser vs. layouter is central to this refactor. To emphasize the importance of this, I'll give a word from experience. I have worked on two attempted PRs that touched on this matter, both failed and never posted on GitHub: Resizing the window with formspecs dynamically without reparsing, and CSS-like units, basically em, px, and %.

The problem with dynamic resizing is that the parsing of the formspec string also lays everything out, like getElementBasePos and getRealCoordinate*. After resizing, the original values from the formspec string are unknown. A button might have an X pos of 192 pixels which we can find, but is this new or old coordinates? What is the button's specific layouting in the old system since it varies so much? Even the new system has problems: Labels and checkboxes are positioned by the middle and inventory lists have spacing in between cells. This can't be resolved without rerunning the parser since that is the layouter.

CSS-like units have the same problem because they need access to parserData to use things like screensize and current_parent, so parserData needs to be passed all over the place where it doesn't belong. size, position, and anchor are also gigantic problems because they don't have access to imgsize or the rectangle of GUIFormSpecMenu yet because that calculating takes place after the parsing of size and co. but before the parsing of everything else. No good. Parsing and layouting are a big slurpy mess of code all mashed together into one pile of non-extensible junk. Not addressing this in this refactor will probably just result in a prettier pile of non-extensible junk.

Perhaps it is better to just limit this refactor to just rearranging code, but this parsing vs. layouting must be addressed at some point, or formspecs really won't be able to grow.

[ghost]

I generally agree with the above. With that said, I think it's best to continue with the current plan and implement such a parsing/layout split later. While it does mean that some of our new code will go through multiple revisions, I think it's best to start by separating and cleaning things a bit before we change the layout mechanism. Otherwise, we're going to wind up with a very large, messy, and slow to merge PR.

We should do this afterwards, though.

[rubenwardy]

That PR is still in a partial state of progress, the eventual aim is for ParserState to only contain parser-time stuff - as its name says. You can't do everything in one go, you have to do it progressively

I do intend to add support for GUI layouting in the future, but that's not the point of this issue.

I define formspecs as strictly the domain-specific language, they will need to be deprecated one day. The point is to separate formspec-related stuff from general GUI stuff, so that an alternative format can be introduced. Perhaps this is a lost cause, and the code should just be archived and this issue closed

[ghost]

Nothing lost about it imo. It's just a big hill to climb.

[v-rob]

Yes; I suppose it would be better as a separate PR. I was a little aggressive in promoting my idea and didn't mean to diminish the importance of or work put into this refactor; sorry.

It's not a lost cause though. Coding is a large hurdle, but by no means impossible. Getting a consensus on a new format for formspecs is probably the harder thing since everyone and their brother has their own idea on what it should be. But this isn't something being doing alone. It's already been shown that there are multiple people willing to help and have helped with previous PRs. There are two core devs willing to work on formspecs (yourself included, of course), so PRs can get merged. It will only be impossible if everyone gives up hope.

[v-rob]

Here's my specific roadmap for the first parts of the refactor: https://forum.minetest.net/viewtopic.php?p=379528#p379528

[pyrollo]

About using XML : If moving from formspec strings, using Lua tables would then be a better choice. Tables have hierarchy and could include functions that could be used as event handlers (onClick, onClose, onMouseOver…) like in many other MT definitions.

Anyway, changing GUI specification format is kind of risky. But it is obviously a good thing to be prepared to it. It seems to me important to separate parsing from any other stuff (drawing, layout, event).

[rubenwardy]

Yeah, a Lua DSL would be a good choice - changing the format is covered by #6527

[v-rob]

Refactor update: Things are going well, although slower than I expected (I wasn't able to work on it for two days). Specifically, the parsing framework is basically in place, and I've gotten buttons to work with the new code.

Technical aspects: I'm largely using Rubenwardy's proposed design and earlier PR, but I've added my own changes:

• GUIFormSpecMenu sends the formspec string to FormSpecParser (currently, only buttons, but later, the entire string will be sent).
• FormSpecParser splits the formspec into elements (basically split(fs, ']')). For each element, it constructs a FormSpecElement and creates an empty ElementSpec. These it sends to the specific parsing function for the element along with ParserState.
• FormSpecElement splits the element into arguments represented as a FormSpecArgument.
• FormSpecArgument has many parsing member functions very much like StyleSpec. This is separate from FormSpecElement because formspec elements can have positional arguments or properties, so FormSpecArgument makes the same interface for both.
• The parsing function called by FormSpecParser takes the FormSpecElement and translates it into the ElementSpec, taking into account all the specific things for that element.
• ElementSpec is where my refactor most differs from Rubenwardy's. Basically, it's a key-value store a lot like StyleSpec. It has string keys and a union value which can represent everything used in formspecs (numbers, strings, vectors, rects, colors, textures, you name it). This makes a unified internal element format that can be translated from a formspec string or JSON or whatever serialization format you want, even allowing mixing multiple ones, for instance, a JSON main form and an old formspec prepend with only the parser knowing the difference.
• FormSpecParser gives the newly created ElementSpecs to GUIFormSpecMenu, which creates the element based on this nice, simple format. GUIFormSpecMenu will not know about configuration elements like scrollbaroptions or set_focus since the parser will integrate those options into the ElementSpec, allowing GUIFormSpecMenu to only care about creating real elements. Note that the exception is style since integrating that would make it impossible for dynamic formspecs to change an external style element.
• Lastly, two more things: Lots of Doxygen comments and no exceptions used.

So what does the button parsing and creation code look like? It's actually fairly simple:

Code:

Parsing:

bool parse_button(ParserState *state, FormSpecElement *element, ElementSpec *spec)
{
	if (element->checkLength(state->formspec_version, {4}))
			return true;

	spec->setElementType(ELEMENT_BUTTON);

	spec->setVector2df ("pos",   element->arg(0).asVector2df());
	spec->setVector2df ("size",  element->arg(1).asVector2df());
	spec->setWideString("label", element->arg(3).asWideString());

	const std::string &name = element->arg(2).asString();
	spec->setString("name", name);

	if (state.focused_element == name)
		spec->setBool("focused", true);

	if (element->getType() == "button_exit")
		spec->setBool("exit", true);

	return element->hasInvalidArgument();
}

Creation

void GUIFormSpecMenu::createButton(ParserState *state, ElementSpec *espec)
{
	v2s32 pos, size;
	core::recti rect;

	if (state->real_coordinates) {
		pos  = getElementPosition(espec->getVector2df("pos"));
		size = getElementGeometry(espec->getVector2df("size"));
		rect = core::recti(pos, pos + size);
	} else {
		pos = getOldElementPosition(espec->getVector2df("pos"));
		pos.Y += (size.Y * imgsize.Y) / 2;
		size.X = (espec->getVector2df("size").X * spacing.X) - (spacing.X - imgsize.X);

		rect = core::recti(pos.X, pos.Y - m_btn_height,
				pos.X + size.X, pos.Y + m_btn_height);
	}

	if (!state->explicit_size) // TODO: Remove
		warningstream << "Invalid use of button without a size[] element" << std::endl;

	bool is_exit = espec->getBool("exit");

	FieldSpec fspec(
		espec->getString("name"),
		translate_string(espec->getWideString("label")),
		L"",
		258 + m_fields.size()
	);
	fspec.ftype = f_Button;
	fspec.is_exit = is_exit;

	GUIButton *e = GUIButton::addButton(Environment, rect, m_tsrc,
			state->current_parent, fspec.fid, fspec.flabel.c_str());

	e->setStyles(getStyleForElement(is_exit ? "button_exit" : "button",
			fspec.fname, is_exit ? "button" : ""));

	if (espec->getBool("focused"))
		Environment->setFocus(e);

	m_fields.push_back(fspec);
}

So, what is left to be done? Mainly, migrating all the elements to the new format. This is fairly simple, as seen with the above code examples, but also necessitates moving lots of stuff into ParserState. After this, StyleSpec will be changed to inherit from ElementSpec so that it doesn't have to worry about parsing either and allowing it to use the new serialization format as well. Finally, make a PR and have lots of reviewing and testing!

The branch if you want a look at it: https://github.com/v-rob/minetest/tree/refactor-parse

[v-rob]

There seems to be some conflict on how the refactor should be designed and carried out, and I want to resolve these before I do anything else. So here goes:

First, to refactor or to not refactor:

1. Refactoring consists of taking a specific aspect of the existing code and separating it. It is impractical to do everything at once, so this requires making much glue code to make everything work properly until the next refactor. Overall, this would likely take longer, but the benefits would be immediate.
2. Starting from scratch would consist of leaving the existing code as-is and developing a new GUI alongside this code. The benefit is that there is complete control over how the code is organized, how it works, and everything. Things like a dynamic GUI, layouting systems, and JSON serialization can be built in (or at least considered) from the start. The downside is that the entire GUI has to be fleshed out before it can be used at all and the old code removed.

Secondly, parsing design:

1. The original idea was to completely extract parsing from the actual element code. As a result of cross-references all over the place in formspecs, this is necessary to have clean code with formspecs. For instance, scroll_container relies on a separate scrollbar somewhere else in the formspec, so after all elements have been created, the scrollbar is searched for and attached to the container. Therefore this allows for two possibilities:
   1. Allow the parser to create the elements. This really isn't separating parsing at all, but keeping it linked together.
   2. Have the parser create element definitions and send them back after all parsing is done. This has two possibilities:
      1. Create a class for each element describing the parsed element. This won't play well with dynamic formspecs as any single member in each class could be changed. Having every property being set again when the formspec is updated is a bad idea and similar to what we have now. Also, if a property can contain multiple values (e.g. either a string or a list of strings), then cumbersome unions must be used.
      2. Create a heterogeneous container that can hold any value. When the formspec is updated, this container will only contain the changed properties, and these properties can be of multiple types.
2. The other option (and this is only feasible if we start from everything from scratch) is to forgo formspec code entirely and solely use JSON. If old formspec code is shown with minetest.show_formspec, it is translated to the JSON serialization format in Lua on the server and then sent to the client. This effectively separates the parsing from the code since JSON needs basically no parsing and Lua is very well suited to working with heterogeneous data since it's dynamically typed. The downfall of this solution is that this won't play well between old servers and new clients or vice-versa, necessitating Minetest 6.0.0, although it can be in development alongside the existing formspec code long before that. Unfortunately, this would not work since there are also node meta formspecs, built in formspecs (death and Esc menu), and the main menu which use different mechanisms, not all of which are exposed to Lua.

Third, Irrlicht usage; I think it is almost certain that we will use Irrlicht as the GUI system while we are redesigning it, but there are still choices to be made on how we use it:

1. Each element is stored raw with no elements managing it. Exterior element things, like event data, are stored separately from the element (this is what is used currently; FieldSpec basically contains everything else).
2. Elements are wrapped in another element class. This class handles events, element name, styling, extra data, etc. This allows the GUI system to be changed from Irrlicht to something else with ease, although most (all?) Irrlicht element code will still be copied into our codebase to apply bugfixes, styling, and features. This would be easiest to do by starting from scratch.
3. Copy all Irrlicht element code directly into Minetest and incorporate everything into it. This inextricably links the GUI system to Irrlicht and would require a complete rewrite (again) to change the GUI system to something else.
4. Using another library or writing our own GUI system are also possibilities, but this would slow GUI development extremely.

I personally would opt for a complete rewrite as opposed to a refactor, but doing this requires formalizing the JSON format, which needs discussion. For parsing, I would choose element definitions with heterogeneous containers, which is what my refactor was currently using. As for Irrlicht usage, I think a wrapper class is the no-brainer, although I think others will likely argue for a different library.

I would like feedback on what others think should be done before I move forward.

[v-rob]

A large-scale formspec refactor is no longer relevant. Replacement is the way things are going.