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

GUI: Checkbox widget #168

Closed Hopson97 closed 4 years ago

Hopson97 commented 4 years ago

Describe your suggestion

Simple true/false checkbox thing for GUIs could come in very handy

Implementations ideas [optional]

Same as other GUIS:

  1. Create a gui::CheckBoxWidget class

  2. Add function to the gui::Overlay class for adding this to the gui

  3. Add Lua API for it in the lua_gui_widget_api.cpp file

georgew79 commented 4 years ago

So, I'm fairly new to this project, and so I had to spend a bit familiarizing myself with the structure of the program, but I've developed what I believe to be a solution.

I followed your advice to create a gui::CheckBoxWidget class, and added functionality in the gui::Overlay class. I've also tested the creation and deletion of the checkbox in lua code itself. As for right now, I have a sample one on the main menu just for testing... (see the image, though please note my computer art skills are lacking).

The gui::CheckBoxWidget class is, as I noticed during its creation, extremely similar to the button widget. As such, I have a few implementation questions, if I may.

First of all, about its usage, I have implemented the "setChecked" and "getChecked" methods in my gui::CheckBoxWidget class. I have also implemented them in the gui_widget_api.cpp file so that they can be accessed in lua code. Beyond this, however, is there any other way you wish the information for a check box to be accessed? Moreover, should the buttons have any text on them? Or will that be handled elsewhere?

Second, I added in a few options to edit the position and size of a widget in the LUA code. A giant button sized widget seemed a bit off to me. As such, I left the initBasicWidget function in the LUA code for now, but I also added an initBasicWidgetChangeable function, to allow for size and position displacement.

Third, I also added the initTextlessWidget and initTextlessWidgetChangeable functions. I, personally, do not consider this to be the best solution, as these two functions are carbon copy the same as BasicWidget variants, just without the text size option. Thus, if you're okay with it, I'd be more than happy to edit the initBasicWidget functions to make them more general (as a checkbox is a perfect example of a widget that may not necessarily have text).

Fourth, I'm not gonna lie, I'm fairly new to github. As such, I don't completely know how to show you all the code without opening a pull request... though, to my knowledge, that requires a new branch, which I can't create. Is there another way I'm missing?

I can't think of any other comments I had. I know I noticed a fatal error occur when I tried to take a windows screenshot with the window for the game open and in focus, so, if you're okay with it, I'd be more than happy to open an issue about that. The implementation is very very simple, so I'd love some feedback on what might be needed next for this checkbox widget, especially with the whole position displacement, and size editing, features I added.

Thank you for all your time! This is a wonderful project, and I'm looking forward to doing all I can!

Screenshot (17) Screenshot (18)

Hopson97 commented 4 years ago

Hey @BinaryDrag0n, thanks for your interest and contribution to the project!

Don't worry about the art of the checkbox, for now we should only be concerned about the functionality of it. From what I can see in the screenshots, and from what I have read here, it seems you have done a pretty good job of implementing this widget

To go through your points:

  1. The getChecked and setChecked could be squashed down into a single property, such as checkBoxApi["checked"] = sol::property(&CheckBoxWidget::getChecked, &CheckBoxWidget::setChecked).

This should work as when using the function sol::property (docs here: https://sol2.readthedocs.io/en/latest/api/property.html), the first parameter is a getter function, but it also allows a second parameter as a setter.

This means when using a checkbox via the Lua code, you can do this:

local checkbox = overlay:addCheckbox()
checkbox.checked = true --calls CheckBoxWidget::setChecked
print(checkbox.checked) --calls CheckBoxWidget::getChecked, prints true

As for whether or not they need text, IMO they should have a label, similar to how the TextboxWidget does. This could either be above the checkbox or to the left, it's up to you and whatever you feel is best :) Doing this also means the initTextlessWidget functions may not be needed.

  1. Having a giant checkbox probably isn't the most common thing. I guess you are talking about initBasicWidget function in the stack_menu.lua, which uses a fixed value for setting the size of widgets, adding a new function is definitely the right thing to do here.

  2. Covered by 1

  3. Ah glad you have chose this as your first project to contribute to :) I noticed on your profile you have forked this project. I believe if you push the code to your own fork master branch, then I am able to see the code by going to your profile. Doing this should also allow you to create a pull request, there is more information on this topic here: https://help.github.com/en/enterprise/2.16/user/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request-from-a-fork

As for the fatal error, if you are able to recreate that issue then I am more than happy for you to open an issue about this.

Once again, thanks for the contribution so far, I am glad you are enjoying the project!

georgew79 commented 4 years ago
  1. Sounds good. I'm still a bit new to LUA, so I didn't know about that feature. Thanks for the advice! I'll implement that.

  2. I'll get to work on implementing a new function, and let you know when I've got that done. I'll try and set it up to be fairly general (and you're right, I forgot to specify the file, stack_menu.lua was indeed what I was talking about)

  3. Ah okay, sounds good! I'll upload it then once I've made the edits above, and let you know. Issue: I will try and recreate it soon, and when I go to upload my progress to my fork I'll have a write-up of everything about the issue I can think of.

Thanks for all your quick feedback!

georgew79 commented 4 years ago

So I've updated my repository, and put the changes I made under the checkbox-api branch in my repository, if you'd like to peak a look at some point before I put the pull request up. I saw, however, that there have been some updates, so I'm re-downloading all the source code and testing. I should be able to have a pull request open soon. Just thought I'd leave in a little update.

Hopson97 commented 4 years ago

Thanks for the update, feel free to make a PR whenever you feel fit, and then I'll take a better look at what you have done. Cheers!

georgew79 commented 4 years ago

Of course! I'm glad to be working on this project! The pull request is here: #183