Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add formspec toolkit and refactor mainmenu to use it #1241

Merged
merged 1 commit into from
May 16, 2014

Conversation

sapier
Copy link
Contributor

@sapier sapier commented Apr 20, 2014

it's quite a lot of copy n paste, major changes are in fst subfolder,
the formspecs itself haven't been changed except of button bar which now does support scrolling for those of you having to many games installed

fstk api is first draft, if you have suggestions how to improve you're welcome

@rubenwardy
Copy link
Member

This makes the main menu much cleaner.

--
--You should have received a copy of the GNU Lesser General Public License along
--with this program; if not, write to the Free Software Foundation, Inc.,
--51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lua files don't normally have full license headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lua files have grown to an extent where headers are reasonable

@sapier
Copy link
Contributor Author

sapier commented Apr 29, 2014

refused due to silly naming dogmas closing

@sapier sapier closed this Apr 29, 2014
dofile(scriptpath .. DIR_DELIM .. "fst" .. DIR_DELIM .. "dialog.lua")
dofile(scriptpath .. DIR_DELIM .. "fst" .. DIR_DELIM .. "tabview.lua")
dofile(scriptpath .. DIR_DELIM .. "fst" .. DIR_DELIM .. "ui.lua")
dofile(scriptpath .. DIR_DELIM .. "ngmm" .. DIR_DELIM .. "common.lua")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ngmm and fst shouldn't be so abbreviated, you can't tell what they are without looking at the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a folder formspectoolkit as well as nextgenerationmainmenu is nonsense too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! So that's what ngmm means. I'd suggest formspec_toolkit. I don't see why there's a separate "next generation" menu folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what ide you use, not even if you use them but having foldernames as long as formspec_toolkit does waste tremendous amount of screen space for nothing.

the ng is because it's a total refactoring and not same as previous, if you wanna move all the mainmenu code to a separate folder you're free to do so.

@sapier
Copy link
Contributor Author

sapier commented Apr 29, 2014

Wontfix issues:
-renaming of folders, written in total they're gonna be too long
-usage of metatables:
1) it's slightly slower due to metatables beeing second chance references only
2) messing up a single element will (in worst case) destroy all objects

@sapier sapier reopened this Apr 29, 2014
@sapier
Copy link
Contributor Author

sapier commented May 10, 2014

deleted comments not related to this commit.
PS everyone who believes this to be rude ... maybe, but that discussion isn't about mainmenu, if you want to rewrite it do it, this one is only a cleanup.

Well I've been told not to delete offtopic comments: here we go:

--------xxyz:--------
👎, formspecs should just die

--------sfan5:--------
+1

--------ShadowNinja:--------
I agree that formspecs should be replaced, as does kwolekr.
Here's a design for it that I came up with (elements are auto-placed by the engine, like in HTML):

local formspec = {
        width = 5,  -- Optional
        height = 5, -- Optional
        {type="button", name="tstbtn", text="Test"},
        "\n",
        "Here are some elements in tabs:\n",
        {
                type = "tabs",
                tabs = {
                        ["Table"] = {
                                type = "table",
                                elements = {
                                        {"Test 1", ">>>>>>>>>", {type="button", name="clickme", text="Click me!"}},
                                        {{type="image", src="test.png"}, "/////////", "^^^^^^^^^"},
                                },
                        },
                        ["List"] = {
                                type = "list",
                                elements = {
                                        "These are a few",
                                        "lines of text that",
                                        "you can select.",
                                        {
                                                type = "button",
                                                name = "list button"
                                                text = "Along with a button.",
                                        },
                                },
                        },
                },
        },
}

This could be sent over the network as JSON (or Lua, via serialize()).
This would break compatibility though, at least without a lot of conversion code.

Another option would be using a already proven format, it may be hard to convert it to a Irrlicht GUI though.

--------sfan5:--------
I would prefer placing elements with (x, y) coordinates in pixels, instead of Inventory fields (Who came up with this?!).
I agree that storing formspecs in lua tables is the best way to store them.

--------MetaDucky:--------
I always found the inventory-field-alignment being a convenient grid when creating a form layout, pixel alignment otoh is not resolution independant - wouldn't this be a step backward with the ongoing efforts on other ends of the GUI to make it all adjust to DPI and screen resolution?

I like the idea of Lua tables for forms, for mods it would be more convenient to update parts without recreating a huge form string.
The above design proposal would need some work though (don't use hash indices in Lua tables when order is important, as they may appear in arbitrary order), and a pure auto layout may probably not be sufficient for more sophisticated GUIs (imagine HTML without CSS).

--------ShadowNinja:--------
@MetaDucky: The position is optional. Pixel positions are't optimal, but inventory slots aren't good either because you don't know their size and might end up with overlapping elements or elements that are too far apart, especially if the user doesn't use the default font size. Auto-placing ought to work pretty well, we don't need HTML+CSS-level position control.

@MetaDucky
Copy link

Ah yes, using self looks much more like Lua now ;) (I think I still spotted a this here and there...)

But, I think you should reconsider using metatables: You had some speed comparison referenced in one of the comments (can't find it anymore right now), which IMO is deceiving about the actual performance impact. Of course, with a hundred million calls one can measure that the extra metatable access doesn't come for free: With your test, the metatable version runs about 20% slower (ymmv).
But consider:

  1. Since func() in your test does almost nothing, these are only 20% of the call and loop overhead: add just 2 lines of code to func(), then it quickly reduces to 4% and less, where it becomes mostly neglible for what it's used for (similarly, you wouldn't avoid the use of virtual functions in C++ just because they may be slightly slower due to the extra vtable lookup)
  2. OTOH, you're spending all the speed win with table initialisation during creation (with a guessed equivalent of a couple dozen method calls in some cases), and creating new closures (for the default methods) every time - this further reduces the performance gain, or could makes it worse for some elements.
  3. Using prototypes would make this much cleaner, understandable, and maintainable (IMO) - the whole concept almost screams for this pattern...

@sapier
Copy link
Contributor Author

sapier commented May 11, 2014

MetaDucky, the only non "style" relate reason for metatables was "it's faster", I prooved this reason is wrong.
And as always "style" is a personal matter, I don't consider metatables to be a more clean way of doing it imho it looks like adding a concept to a language not designed for that concept. Well c++ is almost everytime doing this.
I did use that benchmark for good reason, Usually you don't do a speed test for a f1 car on country road ;-). And yes I agree initialization might be faster on using metatables (didn't benchmark this, so it could be wrong too) BUT, fstk is all about complex menus, therefore they're supposed to to be create once and reused everytime they shall be shown.

And to be honest, I even consider this performance discussion to be absolutely nonsense, none of those things will have any practical difference. But as this was the only technicaly valid reason I tested it anyway.
I'll not spend additional time on falsifying other invalid reasons chosen to support style issues.

Edit1:
Everyone is talking about replacing formspec, why spend additional time to make sure to use any language speciality lua offers? And additionally do you really believe creating a solution more maintainable for two or three persons but unreadable for most of lua developers is better then creating a plain simple understandable solution?

@sapier
Copy link
Contributor Author

sapier commented May 11, 2014

fixes #1284 too

@MetaDucky
Copy link

none of those things will have any practical difference

Performance-wise, that was what I was trying to say.

And additionally do you really believe creating a solution more maintainable for two or three persons but unreadable for most of lua developers is better then creating a plain simple understandable solution?

I may not be too way off when I assume that most Lua developers will find the prototype pattern a plain simple understandable solution - but when you say that it's not worth the additional time then it is irrelevant anyway - I was thinking that was at least a part of the idea (still a bit confused about code quality requirements).

@sapier
Copy link
Contributor Author

sapier commented May 11, 2014

MetaDucky looking at most mod's code you're most likely way of reality. And to be honest I don't think metatables to be more understandable too. Especally as they don't exactly fit to c++ vtable concept too.

Code quality requirements? You're kidding, you're really questioning this commit as of code quality especially comparing it to current code?

Edit1: And btw It's not gonna be you or ShadowNinja having to fix the bugs caused by things like that, by now it's always been me to fix the issues as everyone else told "fix it"

@MetaDucky
Copy link

Sorry - my confusion is about the different standards applied on different ends of the project, where things are rather left broken because of abstruse structural requirements for contributions on one end, and "don't bother me with that"-attitudes on the other end. This makes it pretty hard to provide contributions in form that is helpful, or at least, expected, if there isn't even a consensus amongst the maintainers about what they want. But that's going off-topic (again), and I don't really need far fetched reasons that justify a particular decision, either - I won't bother you again with it.

@sapier
Copy link
Contributor Author

sapier commented May 12, 2014

MetaDucky you claim this to be broken, for what I see it works just fine. The only thing is you/ShadowNinja believe your style of coding is the one and only correct way of doing.
No matter I'm tired of fighting ppl causing useless work.
As this hopefully is gonna be my last contribution to mainmenu anyway I'll use your damn metatables ... and in case of bugs related to those changes I'll forward any requests to ShadownNinja and you.

Edit1:
""don't bother me with that"-attitudes on the other end" ... I can't in any way understand what you want to tell with this as of my pov questioning this commit about code quality is more then picky

Fix crash on using cursor keys in client menu without selected server
Add support for non fixed size tabviews
@sapier sapier merged commit c398456 into minetest:master May 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants