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

feat: add test for construct using unit struct #328

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mgi388
Copy link

@mgi388 mgi388 commented Feb 26, 2025

Pushing this PR up so we can document how to construct unit structs. The first commit of this PR will fail with:

---- script_test - lua - /construct/construct_unit_struct.lua ----
error: Error in function construct : Error converting argument 2: Value mismatch, expected: std::collections::HashMap<alloc::string::String, bevy_mod_scripting_core::bindings::script_value::ScriptValue, std::collections::hash_map::RandomState>, got: [].
Context:
stack traceback:
        [C]: in function 'construct'
        [string "crates/languages/bevy_mod_scripting_lua/src/l..."]:2: in main chunk

Not sure how to get this to work without passing dummy values like this:

local type = world.get_type_by_name("UnitStruct")
local constructed = construct(type, {
    dummy = true
})

assert(constructed ~= nil, "Value was not constructed")

@makspll feel free to push to my PR if you have a fix. Hopefully this test documents this for future users.

@makspll
Copy link
Owner

makspll commented Feb 26, 2025

Ah you've hit an interesting case!
At the moment ScriptValue has both a Vec and a Map variant,
Now during conversion from lua, mlua has to know the target type to convert to, the way it currently does that, is by looking at the type of keys in your table.

Obviously lua doesn't actually have a distinction between maps and lists, which is the root cause here!

One way to deal with this would be to introduce a map({..}) constructor, which explicitly takes in either a Vec or a HashMap.
Another way would be to introduce a new FromScript implementing primitive ListOrMap, I guess you can already do this with Union, and use that instead of Map or List everywhere, but I think the map constructor way is less restrictive on binding authors

@makspll
Copy link
Owner

makspll commented Feb 26, 2025

Once #329 merges this should work

@mgi388
Copy link
Author

mgi388 commented Feb 26, 2025

Nice! Does the PR need a Rhai test too?

@makspll
Copy link
Owner

makspll commented Feb 26, 2025

Yes that would be appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants