search

flamendless / Slab

An immediate mode GUI for the Love2D framework.
MIT License
295 stars 25 forks source link

[IMPORTANT - NEED FEEDBACK] Major refactor, improvements, and optimization for v1.0.0 #127

Open flamendless opened 2 years ago

flamendless commented 2 years ago

Refactor and improvements include:

After refactoring:

NOTE that these are internals thus wont be breaking changes for the users.

Im seriously thinking of changing the fields passed in table/option to widgets from PascalCase to snake_case to make it uniform, this would however lead to breaking changes and users will have to update all of their code to match the new casing. What do you users of Slab think of this? Please feel free to share your thoughts :)

Example of the change would be:

Slab.BeginWindow("id", {Title = "PascalCase Title", X = 0, Y = 0, AutoResizeWindow = true)})
--to
Slab.BeginWindow("id", {title = "snake_case title", x = 0, y = 0, auto_resize_window = true})

Development of v1.0.0 is taking a long break since I have been really busy with work + 3 side projects.

The only thing we are missing for this major release is some functionalities are not working anymore:

Tested widgets/UIs:

It would be great if someone can do a full test and list here the things that are broken so that other people can also help with the fixing. Thank you!

BREAKING CHANGES

The following now have the function signature (id, selected, opt)

Example:

--before
if Slab.ComboBox("combobox", {Selected = "Apple"}) then end
--after
if Slab.ComboBox("combobox", "Apple"}) then end
youssefsahli commented 2 years ago

snake_case would be better since a lot of libs are using it(penlight, batteries), this wouldn't break code style too much.. Others use camelCase though

flamendless commented 2 years ago

For existing users it would, we would have to change a lot of the fields we pass as options to the widgets from PascalCase to snake_case (see example snippet above)

omi-donlimit commented 2 years ago

As suggested in Discord by pakeke. We could add a option that either errors on PascalCase usage or prints out warning about it that it will be deprecated in future version.

frissyn commented 2 years ago

Being able to use different fonts for different Slab.Text(...) calls would be nice, or just changing the size at least.

flamendless commented 2 years ago

Being able to use different fonts for different Slab.Text(...) calls would be nice, or just changing the size at least.

can you show a minimal code example of how you suggest the improvements in the API would look like so i can get a better grasp of it?

frissyn commented 2 years ago

can you show a minimal code example of how you suggest the improvements in the API would look like so i can get a better grasp of it?

@flamendless Currently, all text rendering is used with the last font object pushed to the font stack. It'd be really beneficial to be able to change what font is used for different text calls. Something like this:

function love.load(args)
  Slab.Initialize(args)

  -- push a new font
  local font = love.graphics.newFont("_fonts/myfont.otf", 16)
  Slab.PushFont(font)

  -- create another font
  font2 = love.graphics.newFont("_fonts/otherfont.otf", 16)
end

function love.update(dt)
  Slab.Update(dt)

  -- use last font on stack
  Slab.Text("test")

  -- use another font
  Slab.Text("test", {Font = font2})
end
flamendless commented 2 years ago

can you show a minimal code example of how you suggest the improvements in the API would look like so i can get a better grasp of it?

@flamendless Currently, all text rendering is used with the last font object pushed to the font stack. It'd be really beneficial to be able to change what font is used for different text calls. Something like this:

function love.load(args)
  Slab.Initialize(args)

  -- push a new font
  local font = love.graphics.newFont("_fonts/myfont.otf", 16)
  Slab.PushFont(font)

  -- create another font
  font2 = love.graphics.newFont("_fonts/otherfont.otf", 16)
end

function love.update(dt)
  Slab.Update(dt)

  -- use last font on stack
  Slab.Text("test")

  -- use another font
  Slab.Text("test", {Font = font2})
end

Im not too keen with the stack approach as it could be harder to maintain and debug for the user. I prefer passing font as arg in the table param of Slab.Text. I'll add that in the next major release v1.0.0 :)

frissyn commented 2 years ago

Sounds good! If you're planning on deprecating the Slab.PushFont(...) function, will there be some other way to set a "default" font, so the {Font = ...} option doesn't have to be passed everytime?

flamendless commented 2 years ago

Sounds good! If you're planning on deprecating the Slab.PushFont(...) function, will there be some other way to set a "default" font, so the {Font = ...} option doesn't have to be passed everytime?

I wont be deprecating any API any time soon.

flamendless commented 2 years ago

Update: ComboBox will have a breaking change in API. Previously:

local selected = "test"
local opt = {Selected = selected} --+ more combo box configuration
if Slab.BeginComboBox("id", opt) then
    --more code
    Slab.EndComboBox()
end

In v1.0.0 release:

local selected = "test"
local opt = {} --contains more combo box optional configuration
if Slab.BeginComboBox("id", selected, opt) then
    --more code
    Slab.EndComboBox()
end

This is to optimize and save table allocations when just using the default ComboBox by not having to create garbage table just for the necessary Selected option.

Furthermore, I would probably change other API to extract necessary option(s) from the table as a required parameter to the function itself.

frissyn commented 2 years ago

I wanted to test if v1.0.0 was in a working alpha state for me to use, so I cloned the repository as a submodule and tried to run one of my existing projects that use Slab, and I ran into a require error:

Error: lib/Slab/Internal/Input/Mouse.lua:32: module 'lib.SlabInternal.Core.TablePool' not found

The require line at Internal/Input/Mouse.lua#L32

should be:

local TablePool = require(SLAB_PATH .. ".Internal.Core.TablePool")

(a . was missing from "Internal.Core.TablePool")

Simple syntax error, but after fixing that and converting all my options to snake_case, I ran into another isuue that doesn't have as simple as a fix:

Error: Syntax error: lib/Slab/Internal/UI/Input.lua:1190: function at line 766 has more than 60 upvalues

Looks like v1.0.0 isn't use-able quite yet, but no rush, just thought I'd point these things out to you 👍🏽

flamendless commented 2 years ago

Weird. The path doesn't error for me. Looking at the latest commit. It's not missing the .

local TablePool = require(SLAB_PATH .. "Internal.Core.TablePool")

converting all my options to snake_case

this hasn't been implemented yet in the v1.0.0 branch.

Error: Syntax error: lib/Slab/Internal/UI/Input.lua:1190: function at line 766 has more than 60 upvalues

This should be fixed with the recent commit #ec1ef14d67fa99e699497d7a7754daab5def9264 Maybe by the time you tested the branch it wasn't still pushed.

Looks like v1.0.0 isn't use-able quite yet, but no rush, just thought I'd point these things out to you 👍🏽

Indeed, I have refactored each module already though, I just need to test and fix errors that came with the improvements.

Thanks for testing :)

flamendless commented 2 years ago

So far with the changes, no more error. The only issue is not all of the widgets are drawn (SlabTest). It seems that v1.0.0 release is coming close!

(I try to squeeze in slab development with my free time during the weekends so i don't have that much time like before haha)

halsten-dev commented 2 years ago

With v1.0.0 branch I have this error at start :

Error: libs/slab/Internal/Input/Mouse.lua:33: module 'libs.slabInternal.Core.TablePool' not found: no field package.preload['libs.slabInternal.Core.TablePool'] no 'libs/slabInternal/Core/TablePool' in LOVE game directories. no file 'libs/slabInternal/Core/TablePool' in LOVE paths. no file './libs/slabInternal/Core/TablePool.lua' no file '/usr/local/share/luajit-2.1.0-beta3/libs/slabInternal/Core/TablePool.lua' no file '/usr/local/share/lua/5.1/libs/slabInternal/Core/TablePool.lua' no file '/usr/local/share/lua/5.1/libs/slabInternal/Core/TablePool/init.lua' no file './libs/slabInternal/Core/TablePool.so' no file '/usr/local/lib/lua/5.1/libs/slabInternal/Core/TablePool.so' no file '/usr/local/lib/lua/5.1/loadall.so' no file './libs.so' no file '/usr/local/lib/lua/5.1/libs.so' no file '/usr/local/lib/lua/5.1/loadall.so' stack traceback:

[C]: in function 'require'
libs/slab/Internal/Input/Mouse.lua:33: in main chunk
[C]: in function 'require'
libs/slab/Internal/UI/Dock.lua:31: in main chunk
[C]: in function 'require'
libs/slab/Internal/UI/Window.lua:35: in main chunk
[C]: in function 'require'
libs/slab/Internal/UI/LayoutManager.lua:35: in main chunk
...
[C]: in function 'require'
libs/slab/init.lua:30: in main chunk
[C]: in function 'require'
main.lua:18: in main chunk
[C]: in function 'require'
[love "boot.lua"]:316: in function <[love "boot.lua"]:126>
[C]: in function 'xpcall'
[love "boot.lua"]:355: in function <[love "boot.lua"]:348>
[C]: in function 'xpcall'

Am I doing something wrong ?

flamendless commented 2 years ago

With v1.0.0 branch I have this error at start :

Error: libs/slab/Internal/Input/Mouse.lua:33: module 'libs.slabInternal.Core.TablePool' not found: no field package.preload['libs.slabInternal.Core.TablePool'] no 'libs/slabInternal/Core/TablePool' in LOVE game directories. no file 'libs/slabInternal/Core/TablePool' in LOVE paths. no file './libs/slabInternal/Core/TablePool.lua' no file '/usr/local/share/luajit-2.1.0-beta3/libs/slabInternal/Core/TablePool.lua' no file '/usr/local/share/lua/5.1/libs/slabInternal/Core/TablePool.lua' no file '/usr/local/share/lua/5.1/libs/slabInternal/Core/TablePool/init.lua' no file './libs/slabInternal/Core/TablePool.so' no file '/usr/local/lib/lua/5.1/libs/slabInternal/Core/TablePool.so' no file '/usr/local/lib/lua/5.1/loadall.so' no file './libs.so' no file '/usr/local/lib/lua/5.1/libs.so' no file '/usr/local/lib/lua/5.1/loadall.so' stack traceback: [love "boot.lua"]:345: in function <[love "boot.lua"]:341> [C]: in function 'require' libs/slab/Internal/Input/Mouse.lua:33: in main chunk [C]: in function 'require' libs/slab/Internal/UI/Dock.lua:31: in main chunk [C]: in function 'require' libs/slab/Internal/UI/Window.lua:35: in main chunk [C]: in function 'require' libs/slab/Internal/UI/LayoutManager.lua:35: in main chunk ... [C]: in function 'require' libs/slab/init.lua:30: in main chunk [C]: in function 'require' main.lua:18: in main chunk [C]: in function 'require' [love "boot.lua"]:316: in function <[love "boot.lua"]:126> [C]: in function 'xpcall' [love "boot.lua"]:355: in function <[love "boot.lua"]:348> [C]: in function 'xpcall'

Am I doing something wrong ?

This was also an error raised previously which should be fixed right now. Is your v1.0.0 branch up-to-date?

halsten-dev commented 2 years ago

Yes I just fetched it 20 min ago.

For information TablePool.lua is present in the internal.core folder.

flamendless commented 2 years ago

Yes I just fetched it 20 min ago.

For information TablePool.lua is present in the internal.core folder.

I'm confused, is it still not working? I just pulled the latest branch and it's working for me. Based on the error message you've shown, it seems you are still not up-to-date. Run git log and show what it prints :)

halsten-dev commented 2 years ago

I'm on macOS.

The top most result of git log : 

commit c0ccac5d0808260af8b7a5ea00b824cfbc7513b9 (HEAD -> v1.0.0, origin/v1.0.0)
Author: Brandon Blanker Lim-it <flamendless8@gmail.com>
Date:   Tue Apr 19 11:22:36 2022 +0800

    More bug fixes - #127
flamendless commented 2 years ago

I'm on macOS.

The top most result of git log :

commit c0ccac5d0808260af8b7a5ea00b824cfbc7513b9 (HEAD -> v1.0.0, origin/v1.0.0)
Author: Brandon Blanker Lim-it <flamendless8@gmail.com>
Date:   Tue Apr 19 11:22:36 2022 +0800

    More bug fixes - #127

Weird, I just got the error. Sorry. Pushed a fix now :)

halsten-dev commented 2 years ago

You're amazing thanks for your help.

Sadly I have another one right behind. Do you want me to start another issue for each error I find or I just post it right here ?

flamendless commented 2 years ago

You're amazing thanks for your help.

Sadly I have another one right behind. Do you want me to start another issue for each error I find or I just post it right here ?

You may put the errors you find in a single and separate issue :)