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

[Godot 4] Typed arrays support, Enable packed arrays, Workaround for read-only collections & some small fixes. #6

Merged
merged 18 commits into from
Apr 15, 2023

Conversation

Okxa
Copy link

@Okxa Okxa commented Mar 13, 2023

I wanted to use this addon with nested array/dict structures so I fixed some things:

  • Typed arrays are now supported; The type picker disables types other than the specified.*1
  • Enabled packed arrays too, might as well have them editable for consistency.
  • Workaround for arrays/dictionarys sometimes being read-only*2
  • fixed various bugs where addon tried to access null instances.

*1 Aside from disabling user from changing to incorrect type, the code checks for changing items to incorrect types. The type switcher code also had issues because it assumed the button type list was same as engine type list, but the button type list omitted some types, so the index did not match. Fixed this by creating helper func to map selection index to real type.

(As for not hiding the other types completely with typed arrays, it makes this easier to code because the index then corresponds to the list in type_option_button.)

*2 Some arrays & dictionarys reported as being read-only on first use. I implemented a workaround that appends the collection to a new collection and then reassings it back to the original collection. This might be resource intensive on large collections (untested). This issue might be related or not, I am not sure: godotengine/godot#73766

In any case, I had the read-only issue in the test project (with the resources and with a test scene/script), when adding elements to a array the first time. Not after having added elements, even after reload of scene (or the whole engine).

@don-tnowe
Copy link
Owner

Hello. Thank you for the contribution!

Some arrays & dictionarys reported as being read-only on first use

Huge find. Might be an issue with an in-dev feature for another of my plugins. I couldn't edit a value but it showed no error message, did it for you?

the button type list was same as engine type list, but the button type list omitted some types, so the index did not match

But the ID did? When I was testing and changing the type it I see now, when it's created it's the wrong type. Thanks! Extra props for changing the dict to type constants, I forgot I put literals there.

base_property_editor.gd:4

Isn't cast_to the same as @GDScript.convert()? I remember that having an issue with converting the type to the same type (returned a null) but it's only used once here, could just be a check.

@Okxa
Copy link
Author

Okxa commented Mar 20, 2023

I couldn't edit a value but it showed no error message, did it for you?

I noticed the error mainly on arrays, could not add any values to arrays on first time, and the editor showed error:

E Array is in read-only state.
Set typed_arr_init_empty

With further testhing, dictionarys do not seem to be read-only, just cannot add with dict[key] = value to a empty dict, so I changed the check to check for empty instead. e8cf74f

Isn't cast_to the same as @GDScript.convert()? I remember that having an issue with converting the type to the same type (returned a null) but it's only used once here, could just be a check.

I see, I wasn't aware of convert() yet. Seems to be working with it as well so changed it. 70ca2bc (And here it is used only when the types don't match between array and the new value)

@Okxa
Copy link
Author

Okxa commented Mar 21, 2023

while the workaround is "working" there might be bugs because read-only is by design, according to:

I believe collections were made read-only in the editor in 4.0 by design to fix various bugs. This is one of the reasons support for read-only arrays and dictionaries was added.

godotengine/godot#73766 (comment)

So rather up to you if the workaround for arrays should be merged to this.

Anyways, probably should start porting this to engine then, because array editing is as bad rn as dicts, in the stock editor. Unfortunately I do not have time right now to investigate this, could otherwise, even if I do not have much experience yet in the editor code itself.

@don-tnowe
Copy link
Owner

Got to reviewing PRs properly, so now should be merged. Thank you!

Apparently, this is in fact getting ported to engine - not by me, but I'm looking forward to it as well!

@don-tnowe don-tnowe merged commit 9a6f1eb into don-tnowe:godot-4 Apr 15, 2023
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