-
-
Notifications
You must be signed in to change notification settings - Fork 49
feat: Better HashMaps - hashmap iteration and read-access in map_get() #487
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
Closed
Closed
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
b632625
hashmap iteration
aggyomfg fc7d746
map_get via to_dynamic partial reflect
aggyomfg d6deeac
Merge remote-tracking branch 'origin/main' into hashmap_iter
aggyomfg e7e1f37
get_map_clone - some times needs full reflected value (assets, constr…
aggyomfg 9d99dc3
map_get_clone don't need mutable acces
aggyomfg 55724f1
improve iters to iter partial reflect or _clone (full Reflect)
aggyomfg 52e1e14
ipairs_clone
aggyomfg 12865a8
ipairs_clone can be used with map (default lua ipairs can't)
aggyomfg 0bdb879
Merge branch 'main' into hashmap_iter
makspll 96c9156
get rid of leaked abstraction
aggyomfg da7a771
get rid of leaked abstraction 2
aggyomfg bb789b9
use default_get instead of map_get
aggyomfg 9a8a943
set maps via default_set
aggyomfg af9eff7
rm insert binding
aggyomfg 64c0221
fix rhai insert test
aggyomfg 75bded4
hashset support and better magic functions
aggyomfg f476eef
hashset iter
aggyomfg 32484f1
rm duplicate test
aggyomfg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| local res_type = world.get_type_by_name("TestResourceWithVariousFields") | ||
| local res = world.get_resource(res_type) | ||
|
|
||
| local map = res.string_map | ||
|
|
||
| local count = 0 | ||
| local found_keys = {} | ||
|
|
||
| --- Iterate over PartialReflect refs using pairs | ||
| for key, value in pairs(map) do | ||
| count = count + 1 | ||
| found_keys[key] = value | ||
| end | ||
|
|
||
| assert(count == 2, "Expected 2 entries, got " .. count) | ||
| assert(found_keys["foo"] == "bar", "Expected foo=>bar") | ||
| assert(found_keys["zoo"] == "zed", "Expected zoo=>zed") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| let res_type = world.get_type_by_name.call("TestResourceWithVariousFields"); | ||
| let res = world.get_resource.call(res_type); | ||
|
|
||
| let map = res.string_map; | ||
|
|
||
| let iterator = map.iter(); | ||
| let count = 0; | ||
| let found_keys = #{}; | ||
|
|
||
| loop { | ||
| let result = iterator.next(); | ||
|
|
||
| if result == () { | ||
| break; | ||
| } | ||
|
|
||
| let key = result[0]; | ||
| let value = result[1]; | ||
| count += 1; | ||
| found_keys[key] = value; | ||
| } | ||
|
|
||
| if count != 2 { | ||
| throw `Expected 2 entries, got ${count}`; | ||
| } | ||
| if found_keys["foo"] != "bar" { | ||
| throw "Expected foo=>bar"; | ||
| } | ||
| if found_keys["zoo"] != "zed" { | ||
| throw "Expected zoo=>zed"; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| local res_type = world.get_type_by_name("TestResourceWithVariousFields") | ||
| local res = world.get_resource(res_type) | ||
|
|
||
| local map = res.string_map | ||
|
|
||
| local iterator = map:iter_clone() | ||
| local count = 0 | ||
| local found_keys = {} | ||
|
|
||
| local result = iterator() | ||
| while result ~= nil do | ||
| local key = result[1] | ||
| local value = result[2] | ||
| count = count + 1 | ||
| found_keys[key] = value | ||
| result = iterator() | ||
| end | ||
|
|
||
| assert(count == 2, "Expected 2 entries, got " .. count) | ||
| assert(found_keys["foo"] == "bar", "Expected foo=>bar") | ||
| assert(found_keys["zoo"] == "zed", "Expected zoo=>zed") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| let res_type = world.get_type_by_name.call("TestResourceWithVariousFields"); | ||
| let res = world.get_resource.call(res_type); | ||
|
|
||
| let map = res.string_map; | ||
|
|
||
| let iterator = map.iter_clone(); | ||
| let count = 0; | ||
| let found_keys = #{}; | ||
|
|
||
| loop { | ||
| let result = iterator.next(); | ||
|
|
||
| if result == () { | ||
| break; | ||
| } | ||
|
|
||
| let key = result[0]; | ||
| let value = result[1]; | ||
| count += 1; | ||
| found_keys[key] = value; | ||
| } | ||
|
|
||
| if count != 2 { | ||
| throw `Expected 2 entries, got ${count}`; | ||
| } | ||
| if found_keys["foo"] != "bar" { | ||
| throw "Expected foo=>bar"; | ||
| } | ||
| if found_keys["zoo"] != "zed" { | ||
| throw "Expected zoo=>zed"; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| local res_type = world.get_type_by_name("TestResourceWithVariousFields") | ||
| local res = world.get_resource(res_type) | ||
|
|
||
| local map = res.string_map | ||
|
|
||
| local count = 0 | ||
| local found_keys = {} | ||
|
|
||
| -- ipairs_clone on a map returns (index, [key, value]) where value is a list | ||
| for i, entry in map:ipairs_clone() do | ||
| assert(i == count + 1, "Index should be sequential: expected " .. (count + 1) .. ", got " .. i) | ||
|
|
||
| -- entry should be a list with [key, value] | ||
| assert(entry ~= nil, "Entry should not be nil") | ||
| assert(entry[1] ~= nil, "Key should not be nil") | ||
| assert(entry[2] ~= nil, "Value should not be nil") | ||
|
|
||
| local key = entry[1] | ||
| local value = entry[2] | ||
|
|
||
| count = count + 1 | ||
| found_keys[key] = value | ||
| end | ||
|
|
||
| assert(count == 2, "Expected 2 entries, got " .. count) | ||
| assert(found_keys["foo"] == "bar", "Expected foo=>bar, got " .. tostring(found_keys["foo"])) | ||
| assert(found_keys["zoo"] == "zed", "Expected zoo=>zed, got " .. tostring(found_keys["zoo"])) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| local res_type = world.get_type_by_name("TestResourceWithVariousFields") | ||
| local res = world.get_resource(res_type) | ||
|
|
||
| local map = res.string_map | ||
|
|
||
| local count = 0 | ||
| local found_keys = {} | ||
|
|
||
| -- Use pairs_clone to loop over Reflect values | ||
| for key, value in map:pairs_clone() do | ||
| count = count + 1 | ||
| found_keys[key] = value | ||
| end | ||
|
|
||
| if count ~= 2 then | ||
| error(string.format("Expected 2 entries, got %d", count)) | ||
| end | ||
| if found_keys["foo"] ~= "bar" then | ||
| error("Expected foo=>bar") | ||
| end | ||
| if found_keys["zoo"] ~= "zed" then | ||
| error("Expected zoo=>zed") | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| local res_type = world.get_type_by_name("TestResourceWithVariousFields") | ||
| local res = world.get_resource(res_type) | ||
|
|
||
| local iterated_vals = {} | ||
| local iterator = res.vec_usize:iter_clone() | ||
| local result = iterator() | ||
| while result ~= nil do | ||
| iterated_vals[#iterated_vals + 1] = result | ||
| result = iterator() | ||
| end | ||
|
|
||
| assert(#iterated_vals == 5, "Length is not 5") | ||
| assert(iterated_vals[1] == 1, "First value is not 1") | ||
| assert(iterated_vals[2] == 2, "Second value is not 2") | ||
| assert(iterated_vals[3] == 3, "Third value is not 3") | ||
| assert(iterated_vals[4] == 4, "Fourth value is not 4") | ||
| assert(iterated_vals[5] == 5, "Fifth value is not 5") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| let res_type = world.get_type_by_name.call("TestResourceWithVariousFields"); | ||
| let res = world.get_resource.call(res_type); | ||
|
|
||
| let iterated_vals = []; | ||
| let iterator = res.vec_usize.iter_clone(); | ||
|
|
||
| loop { | ||
| let result = iterator.next(); | ||
|
|
||
| if result == () { | ||
| break; | ||
| } | ||
|
|
||
| iterated_vals.push(result); | ||
| } | ||
|
|
||
| assert(iterated_vals.len == 5, "Length is not 5"); | ||
| assert(iterated_vals[0] == 1, "First value is not 1"); | ||
| assert(iterated_vals[1] == 2, "Second value is not 2"); | ||
| assert(iterated_vals[2] == 3, "Third value is not 3"); | ||
| assert(iterated_vals[3] == 4, "Fourth value is not 4"); | ||
| assert(iterated_vals[4] == 5, "Fifth value is not 5"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| local res_type = world.get_type_by_name("TestResourceWithVariousFields") | ||
| local res = world.get_resource(res_type) | ||
|
|
||
| local iterated_vals = {} | ||
| for i, v in res.vec_usize:ipairs_clone() do | ||
| assert(i == #iterated_vals + 1, "Index mismatch: expected " .. (#iterated_vals + 1) .. ", got " .. i) | ||
| iterated_vals[#iterated_vals + 1] = v | ||
| end | ||
|
|
||
| assert(#iterated_vals == 5, "Length is not 5") | ||
| assert(iterated_vals[1] == 1, "First value is not 1") | ||
| assert(iterated_vals[2] == 2, "Second value is not 2") | ||
| assert(iterated_vals[3] == 3, "Third value is not 3") | ||
| assert(iterated_vals[4] == 4, "Fourth value is not 4") | ||
| assert(iterated_vals[5] == 5, "Fifth value is not 5") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| local res_type = world.get_type_by_name("TestResourceWithVariousFields") | ||
| local res = world.get_resource(res_type) | ||
|
|
||
| local iterated_vals = {} | ||
| for v in res.vec_usize:pairs_clone() do | ||
| iterated_vals[#iterated_vals + 1] = v | ||
| end | ||
|
|
||
| assert(#iterated_vals == 5, "Length is not 5") | ||
| assert(iterated_vals[1] == 1, "First value is not 1") | ||
| assert(iterated_vals[2] == 2, "Second value is not 2") | ||
| assert(iterated_vals[3] == 3, "Third value is not 3") | ||
| assert(iterated_vals[4] == 4, "Fourth value is not 4") | ||
| assert(iterated_vals[5] == 5, "Fifth value is not 5") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced about using
to_dynamic, all of of the current API surface avoids nonReflectvalues for a reason, any downcasts (i.e. into_script_ref calls) will fail on dynamic structs etc, this will work for primitives but more complex opaque values which don't have areflect_clone(I think, i.e. opaque values without an explicitreflect(clone), or even anything with areflect(ignore)field) will cause issues.So while this might work individually, but will introduce problems with some bindings.
For example
Ref<T>usestry_downcast_ref::<T>, which will fail if the value came fromto_dynamic, asDynamicStructetc are not concreteReflecttypes. This is why need calls toFromReflectfor getting owned variants of values.Imo the decision to get a dynamic vs non dynamic value should not be made by the script user, as it's a bit of a leaky abstraction. Ideally the scripter should not need to know anything about the internals here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I agree — I just made a few APIs to discuss the idea. So, just to clarify, we want to keep only the variant that I currently named with the _clone postfix, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes exactly