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.

Sign up for GitHub

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

Jump to bottom

Add gui.get and gui.set #8638

Merged
merged 3 commits into from
Mar 13, 2024
Merged

Add gui.get and gui.set #8638

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
702aba8
Adding gui.get and gui.set support MVP1
Jhonnyg Mar 7, 2024
a6c43fc
Review changes
Jhonnyg Mar 13, 2024
7343974
Update docs and tests
Jhonnyg Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 1 addition & 8 deletions engine/gui/src/gui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,6 @@ namespace dmGui
{ dmHashString64(#name ".z"), prop, 2 }, \
{ dmHashString64(#name ".w"), prop, 3 },

struct PropDesc
{
dmhash_t m_Hash;
Property m_Property;
uint8_t m_Component;
};

PropDesc g_Properties[] = {
PROP(position, PROPERTY_POSITION )
PROP(rotation, PROPERTY_ROTATION )
Expand Down Expand Up @@ -133,7 +126,7 @@ namespace dmGui
{ dmHashString64("slice"), PROPERTY_SLICE9, 0xff },
};

static PropDesc* GetPropertyDesc(dmhash_t property_hash)
PropDesc* GetPropertyDesc(dmhash_t property_hash)
{
int n_props = sizeof(g_Properties) / sizeof(g_Properties[0]);
for (int i = 0; i < n_props; ++i) {
Expand Down
9 changes: 9 additions & 0 deletions engine/gui/src/gui_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,15 @@ namespace dmGui
dmVMath::Vector4 CalculateReferenceScale(HScene scene, InternalNode* node);

HNode GetNodeHandle(InternalNode* node);

struct PropDesc
{
dmhash_t m_Hash;
Property m_Property;
uint8_t m_Component;
};

PropDesc* GetPropertyDesc(dmhash_t property_hash);
}

#endif
167 changes: 167 additions & 0 deletions engine/gui/src/gui_script.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,171 @@ namespace dmGui
return 0;
}

/*# gets the named property of a specified gui node
*
* Instead of using specific getters such as gui.get_position or gui.get_scale,
* you can use gui.get instead and supply the property as a string or a hash.
* While this function is similar to go.get, there are a few more restrictions
* when operating in the gui namespace. Most notably, only these propertie identifiers are supported:
*
* - `"position"`
* - `"rotation"`
* - `"scale"`
* - `"color"`
* - `"outline"`
* - `"shadow"`
* - `"size"`
* - `"fill_angle"` (pie)
* - `"inner_radius"` (pie)
* - `"slice9"` (slice9)
*
* The value returned will either be a vmath.vector4 or a single number, i.e getting the "position"
* property will return a vec4 while getting the "position.x" property will return a single value.
*
* @name gui.get
* @param node [type:node] node to get the property for
* @param property [type:string|hash|constant] the property to retrieve
* @examples
*
* Get properties on existing nodes:
*
* ```lua
* local node = gui.get_node("my_box_node")
* 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 •
edited

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 •
edited

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)

{
DM_LUA_STACK_CHECK(L, 1);

Scene* scene = GuiScriptInstance_Check(L);

HNode hnode;
LuaCheckNodeInternal(L, 1, &hnode);

dmhash_t property_hash = dmScript::CheckHashOrString(L, 2);

dmGui::PropDesc* pd = dmGui::GetPropertyDesc(property_hash);
if (!pd)
{
char buffer[128];
DM_LUA_ERROR("property '%s' not found", dmScript::GetStringFromHashOrString(L, 2, buffer, sizeof(buffer)));
Jhonnyg marked this conversation as resolved.
Show resolved Hide resolved
}

Vector4 base_value = dmGui::GetNodeProperty(scene, hnode, pd->m_Property);

if (pd->m_Component == 0xff)
{
dmScript::PushVector4(L, base_value);
}
else
{
lua_pushnumber(L, base_value.getElem(pd->m_Component));
}

return 1;
}

/*# sets the named property of a specified gui node
*
* Instead of using specific setteres such as gui.set_position or gui.set_scale,
* you can use gui.set instead and supply the property as a string or a hash.
* While this function is similar to go.get and go.set, there are a few more restrictions
* when operating in the gui namespace. Most notably, only these propertie identifiers are supported:
*
* - `"position"`
* - `"rotation"`
* - `"scale"`
* - `"color"`
* - `"outline"`
* - `"shadow"`
* - `"size"`
* - `"fill_angle"` (pie)
* - `"inner_radius"` (pie)
* - `"slice9"` (slice9)
*
* The value to set must either be a vmath.vector4, vmath.vector3 or a single number and depends on the property name you want to set.
* I.e when setting the "position" property, you need to use a vmath.vector4 and when setting a single component of the property,
* such as "position.x", you need to use a single value.
*
* @name gui.set
* @param node [type:node] node to set the property for
* @param property [type:string|hash|constant] the property to set
* @param value [type:number|vector4|vector3] the property to set
* @examples
*
* Updates the position property on an existing node:
*
* ```lua
* local node = gui.get_node("my_box_node")
* local node_position = gui.get(node, "position")
* gui.set(node, "position.x", node_position.x + 128)
* ```
*/
int LuaSet(lua_State* L)
Jhonnyg marked this conversation as resolved.
Show resolved Hide resolved
{
DM_LUA_STACK_CHECK(L, 0);

Scene* scene = GuiScriptInstance_Check(L);

HNode hnode;
LuaCheckNodeInternal(L, 1, &hnode);

dmhash_t property_hash = dmScript::CheckHashOrString(L, 2);
dmGui::PropDesc* pd = dmGui::GetPropertyDesc(property_hash);

if (!pd)
{
char buffer[128];
DM_LUA_ERROR("property '%s' not found", dmScript::GetStringFromHashOrString(L, 2, buffer, sizeof(buffer)));
}

bool is_vector4 = dmScript::IsVector4(L, 3);
bool is_vector3 = dmScript::IsVector3(L, 3);
bool is_number = lua_isnumber(L, 3);
Jhonnyg marked this conversation as resolved.
Show resolved Hide resolved

if (!(is_vector3 || is_vector4 || is_number))
{
char buffer[128];
DM_LUA_ERROR("Unable to set property '%s', only vmath.vector4, vmath.vector3 or float values are supported", dmScript::GetStringFromHashOrString(L, 2, buffer, sizeof(buffer)));
Jhonnyg marked this conversation as resolved.
Show resolved Hide resolved
}
else if (pd->m_Component == 0xff && !(is_vector4 || is_vector3))
{
char buffer[128];
DM_LUA_ERROR("Unable to set property '%s', the value must be a vmath.vector4 or a vmath.vector3", dmScript::GetStringFromHashOrString(L, 2, buffer, sizeof(buffer)));
}
else if (pd->m_Component != 0xff && !is_number)
{
char buffer[128];
DM_LUA_ERROR("Unable to set property '%s', vector elements can only be set by numbers", dmScript::GetStringFromHashOrString(L, 2, buffer, sizeof(buffer)));
}

if (pd->m_Component == 0xff)
{
if (is_vector3)
{
Vector3* new_value = dmScript::ToVector3(L, 3);
Vector4 current_value = dmGui::GetNodeProperty(scene, hnode, pd->m_Property);
current_value.setXYZ(*new_value);
dmGui::SetNodeProperty(scene, hnode, pd->m_Property, current_value);
}
else
{
Vector4* new_value = dmScript::ToVector4(L, 3);
dmGui::SetNodeProperty(scene, hnode, pd->m_Property, *new_value);
}
}
else
{
Vector4 current_value = dmGui::GetNodeProperty(scene, hnode, pd->m_Property);
float new_element_value = (float) lua_tonumber(L, 3);
current_value.setElem(pd->m_Component, new_element_value);
dmGui::SetNodeProperty(scene, hnode, pd->m_Property, current_value);
}

return 0;
}

/*# gets the index of the specified node
*
* Retrieve the index of the specified node among its siblings.
Expand Down Expand Up @@ -4421,6 +4586,8 @@ namespace dmGui
{"get_node", LuaGetNode},
{"get_id", LuaGetId},
{"set_id", LuaSetId},
{"get", LuaGet},
{"set", LuaSet},
{"get_index", LuaGetIndex},
{"delete_node", LuaDeleteNode},
{"animate", LuaAnimate},
Expand Down
56 changes: 56 additions & 0 deletions engine/gui/src/test/test_gui_script.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,62 @@ TEST_F(dmGuiScriptTest, TestVisibilityApi)
dmGui::DeleteScript(script);
}

TEST_F(dmGuiScriptTest, TestGuiGetSet)
{
dmGui::HScript script = NewScript(m_Context);

dmGui::NewSceneParams params;
params.m_MaxNodes = 64;
params.m_MaxAnimations = 32;
params.m_UserData = this;
dmGui::HScene scene = dmGui::NewScene(m_Context, &params);
dmGui::SetSceneScript(scene, script);

const char* src =
"local EPSILON = 0.001\n"
"function assert_near_vector4(a,b)\n"
" assert(math.abs(a.x-b.x) < EPSILON)\n"
" assert(math.abs(a.y-b.y) < EPSILON)\n"
" assert(math.abs(a.z-b.z) < EPSILON)\n"
" assert(math.abs(a.w-b.w) < EPSILON)\n"
"end\n"
"local function assert_error(func)\n"
" local r, err = pcall(func)\n"
" if not r then\n"
" print(err)\n"
" end\n"
" assert(not r)\n"
"end\n"
"function init(self)\n"
// Test valid
" local node = gui.new_box_node(vmath.vector3(1, 2, 3), vmath.vector3(4, 5, 6))\n"
" assert_near_vector4(vmath.vector4(1, 2, 3, 1), gui.get(node, 'position'))\n"
" assert_near_vector4(vmath.vector4(4, 5, 6, 0), gui.get(node, 'size'))\n"
" gui.set(node, 'position', vmath.vector3(99, 98, 97))\n"
" assert_near_vector4(vmath.vector4(99, 98, 97, 1), gui.get(node, 'position'))\n"
" gui.set(node, 'position', vmath.vector4(99, 98, 97, 1337))\n"
" assert_near_vector4(vmath.vector4(99, 98, 97, 1337), gui.get(node, 'position'))\n"
// Incorrect input for get
" assert_error(function() gui.get('invalid', 'position') end)\n"
" assert_error(function() gui.get(hash('invalid'), 'position') end)\n"
" assert_error(function() gui.get(node, 'this_doesnt_exist') end)\n"
// Incorrect input for set
" assert_error(function() gui.set('invalid', 'position', vmath.vector3()) end)\n"
" assert_error(function() gui.set(node, 'position', 'incorrect-string') end)\n"
" assert_error(function() gui.set(node, 'this_doesnt_exist', vmath.vector4()) end)\n"
"end\n";
Jhonnyg marked this conversation as resolved.
Show resolved Hide resolved

dmGui::Result result = SetScript(script, LuaSourceFromStr(src));
ASSERT_EQ(dmGui::RESULT_OK, result);

result = dmGui::InitScene(scene);
ASSERT_EQ(dmGui::RESULT_OK, result);

dmGui::DeleteScene(scene);

dmGui::DeleteScript(script);
}

TEST_F(dmGuiScriptTest, TestRecreateDynamicTexture)
{
dmGui::HScript script = NewScript(m_Context);
Expand Down