Hopson97 / open-builder

Open "Minecraft-like" game with multiplayer support and Lua scripting support for the both client and server
https://github.com/Hopson97/open-builder/projects/3
GNU General Public License v3.0
700 stars 80 forks source link

#168 GUI: Checkbox Pull Request #183

Closed georgew79 closed 4 years ago

georgew79 commented 4 years ago

This pull request is to add the necessary material in order to create a simple checkbox for the GUI. The implementation is extremely similar to that of the button widget. If there are any issues with it, formatting, other ideas for implementation, compatibility with the newest updates, etc. please let me know. Moreover, this implementation generalizes the initialization of a basic widget (as much as I could think of), instead of relying on fixed sixes. Note! This request includes a test implementation for the main menu. If this needs to be removed before being merged, please let me know. I left it in for testing, in case somebody wanted to test this manually.

In advance, thank you for everyone's time! Screenshot (22) Screenshot (21)

georgew79 commented 4 years ago

These are all great suggestions! I'll be happy to put these into affect as soon as I'm able. LUA is a fairly new thing for me. I've used python before, but not LUA, so I'm always glad to hear new things about it!

I see what you mean now about the stack menu. You have a very good point, I was just trying to figure out how to perhaps set up a concept of "layers" in the stack menu. In other words, the main menu has a bunch of buttons on it. Each one takes up a "layer" (not implemented program-wise, this is strictly conceptual), and is centered on that layer. However, other widgets, unlike buttons (or perhaps buttons, depending on the layout of that page), may not take the whole layer. It's a little hard to describe, so I've drawn up a little image just to try to make my point. The boxes should be imagined as some widgets (I know my computer art skills aren't great ;), but I just took two seconds to mock up something)

Layers

So, in other words, each layer is about the size of the buttons in use right now (all layers the same size), thus the usage of the self.widgetGap value, but the options presented allow one to create layers with multiple small widgets (perhaps, say, multiple small text-less check boxes, which is an option that needs to be implemented), that way there's greater flexibility with the stack menu... at least in my opinion. So that, say, one layer can be comprised of one singular button, play game, whereas another is compromised of, say, 4 different check boxes organized into one singular layer.

I do realize, however, that that approach would require the y modifier to be locked to a max/min in the initBasicWidget function.... I'd need to implement that.

Of course though, this is completely your project, just offering my opinions. I absolutely understand if this doesn't fit with your view of the project, and I'm more than happy to implement it in a different way :) Speaking of, would you perhaps have another suggestion then for the check boxes? Perhaps if they should even be added in the stack_menu?

All your other suggestions are great! I'm happy to implement them, must've just missed some stuff in my implementation, especially the CMake update. I'm more than happy to work on this project, and I'm glad I can be a help!

Thank you for all your time! Have a wonderful day!

Hopson97 commented 4 years ago

Your drawing makes sense to me. The stack-menu was designed with just buttons in mind, I don't tend to be the best at thinking to far ahead, so I can understand the confusion of how a checkbox would fit into it.

I understand what your mean, probably could add some kind of secondary Lua thing, called a "row menu", of which could then get added to the stack menu in some way.

Eg, the interface from Lua would be like

local row = menu:addRowMenu(3) --adds a row menu with 3 smaller widget slots

--Adds the 3 widgets into the slots somehow while also aligning with the stack menu
row:addCheckbox()
row:addCheckbox()
row:addCheckbox()

For now I am not entirely sure how that would work though, but that is out of the scope of this anyways. So, I wouldn't worry about that for now, and focus only on the checkbox 😄

Thanks for contributing, have a great day you too :)

georgew79 commented 4 years ago

Got it! Thanks for the feedback! I'll carry out those solutions marked above then, and mark them resolved, as well as comment, when they're all done. Thanks!

Hopson97 commented 4 years ago

Final note if that is ok, when it comes to doing commit messages, please state exactly what is being changed.

This is because when people go to code, they can do a git blame to see the commit message for every line. (See picture below as example)

image

This is useful for all sorts of reason, and helps when it comes to debugging etc.

Thanks!

georgew79 commented 4 years ago

Sure! Sorry about that, still new to GitHub... but I'm always happy to learn!

georgew79 commented 4 years ago

All the changes should be committed now, including the removal of the test checkbox (so that purely the implementation of the widget remains). This includes the return to the original format of the initBasicWidget function.

Thanks for all your time! Please let me know if there are any issues, or if I forgot any files (I have two different versions I work from, one for testing, one for updating, so I'm prone to missing updates I need to make). Have a great day!

Hopson97 commented 4 years ago

Apologies for the late reply responding this. Had a look at your code, and seems pretty good to me, so I am more than happy to merge this in now. Thanks!

Hopson97 commented 4 years ago

Also sorry for pushing all the GUI stuff to master, I should have merged this in before doing this, but thanks regardless for adapting your code to it :)

georgew79 commented 4 years ago

No problem! Thank you for your time! Please let me know if there are any issues at all with it

Hopson97 commented 4 years ago

I love it image