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 gui.get and gui.set #8638

Merged
merged 3 commits into from Mar 13, 2024
Merged

Add gui.get and gui.set #8638

merged 3 commits into from Mar 13, 2024

Conversation

Jhonnyg
Copy link
Contributor

@Jhonnyg Jhonnyg commented Mar 7, 2024

Gui scripts can now query/set node properties via gui.get and gui.set:

local node = gui.get_node("my_node_id")
local node_pos = gui.get(node, "position")
local node_pos_x = gui.get(node, "position.x")

gui.set(node, "position", vmath.vector3(1, 2, 3))
gui.set(node, "position.x", 1)

We have also added a new property to the gui namespace euler which is linked with the rotation property. When changing one of them, both will change its underlying data. The important change between them is that the rotation property must be set by quaternion values, and the euler property must be set by degree values. You can set a single component of the rotation or the euler properties as usual:

gui.set(node, "rotation.x", math.rad(45))
gui.set(node, "euler.x", 45)

There are no custom script properties available, you can only get and set the already existing node properties:

"position"
"rotation"
"euler"
"scale"
"color"
"outline"
"shadow"
"size"
"fill_angle" (pie)
"inner_radius" (pie)
"slice9" (slice9)

Fixes #5090

@Jhonnyg Jhonnyg added feature request A suggestion for a new feature engine Issues related to the Defold engine gui Issues related to gui components labels Mar 7, 2024
* local node_position = gui.get(node, "position")
* ```
*/
int LuaGet(lua_State* L)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note about rotation:
For go.get/set we support quaternions for e.g rotation. But for gui when using gui.set_rotation and gui.get_rotation we still use euler angles, so we should probably still support that here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good question.
I believe it will be harder to "fix" later on, if we want ppl to merge their code/functions from GUI into GO space.

So, it might be better to return quaternions here?

Let's ask the others in the team as well!

Copy link
Contributor

@matgis matgis Mar 8, 2024

Choose a reason for hiding this comment

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

Perhaps we could expose euler_rotation as a separate property and have rotation operate on quaternions like we do in other places?

I think we should look over the other properties as well to ensure their values are consistent with their go counterparts. I.e. the same property on a go vs a gui returning Vector3 values vs Vector4 values or similar discrepancies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could expose euler_rotation as a separate property and have rotation operate on quaternions like we do in other places?

If we do so, it's a breaking change then

Currently expected beh. for devs in gui is euler (because this is what gui.get_rotation() and gui.set_rotation() do). I think for now it's better to keep it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not suggesting we change the behaviour of gui.get_rotation() and gui.set_rotation(). I'm simply suggesting that we make the new gui.get(node, "rotation") and gui.set(node, "rotation", value) return and take a quaternion value respectively.

So it would be inconsistent with the existing gui.get_rotation() and gui.set_rotation() functions, but not a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

it feels strange

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, to me it feels like it will feel strange either way (i.e. it won't behave the same as go.get)
Do we have any other votes?

Copy link
Contributor

@AGulev AGulev Mar 12, 2024

Choose a reason for hiding this comment

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

if we do so, we have to introduce euler in this PR (as well as it is in go.*)

Copy link
Sponsor

Choose a reason for hiding this comment

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

Is gui.animate(node, "rotation", ...) still work in euler values?

Sounds a little bit strange, that leads to additional logic if I want to make a wrappers around all these functions (gui.set, gui.set_rotation and gui.animate)

@Jhonnyg Jhonnyg requested review from JCash and britzl March 8, 2024 07:20
Copy link
Contributor

@JCash JCash left a comment

Choose a reason for hiding this comment

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

Looks good.
I'd like to see some restructuring of the Set method

* local node_position = gui.get(node, "position")
* ```
*/
int LuaGet(lua_State* L)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good question.
I believe it will be harder to "fix" later on, if we want ppl to merge their code/functions from GUI into GO space.

So, it might be better to return quaternions here?

Let's ask the others in the team as well!

engine/gui/src/gui_script.cpp Outdated Show resolved Hide resolved
engine/gui/src/gui_script.cpp Outdated Show resolved Hide resolved
engine/gui/src/gui_script.cpp Outdated Show resolved Hide resolved
engine/gui/src/gui_script.cpp Outdated Show resolved Hide resolved
engine/gui/src/test/test_gui_script.cpp Show resolved Hide resolved
@Jhonnyg Jhonnyg requested a review from JCash March 13, 2024 12:06
@Jhonnyg Jhonnyg merged commit a291050 into dev Mar 13, 2024
18 checks passed
@Jhonnyg Jhonnyg deleted the issue-5090-gui.get-set branch March 13, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine Issues related to the Defold engine feature request A suggestion for a new feature gui Issues related to gui components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go.set equivalent in gui namespace
5 participants