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

Make inst_to_dict and dict_to_inst recursive #97244

Closed

Conversation

pafuent
Copy link
Contributor

@pafuent pafuent commented Sep 20, 2024

Fixes #6533

Making GDScript inst_to_dict/dict_to_inst utility functions recursive.
Adding also a new macro to validate the number of the required arguments.

@pafuent pafuent requested a review from a team as a code owner September 20, 2024 17:08
@pafuent
Copy link
Contributor Author

pafuent commented Sep 20, 2024

Please pay special attention to my poorly attempt to document the changes that I did to inst_to_dict and dict_to_inst methods

@pafuent pafuent changed the title Making inst_to_dict/dict_to_inst recursive Making inst_to_dict and dict_to_inst recursive Sep 20, 2024
@pafuent pafuent force-pushed the inst_to_dict_nesting_support branch from e77f3af to 59b04fc Compare September 20, 2024 17:10
@AThousandShips AThousandShips changed the title Making inst_to_dict and dict_to_inst recursive Make inst_to_dict and dict_to_inst recursive Sep 20, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Sep 20, 2024
@pafuent pafuent force-pushed the inst_to_dict_nesting_support branch from 59b04fc to f816784 Compare September 20, 2024 17:14
@pafuent pafuent force-pushed the inst_to_dict_nesting_support branch 2 times, most recently from 8770516 to 5acece9 Compare September 20, 2024 18:26
@Mickeon
Copy link
Contributor

Mickeon commented Sep 20, 2024

If I recall correctly, methods like Array.duplicate() have a hardcoded limit of 256 to prevent the infinite recursion.

@pafuent
Copy link
Contributor Author

pafuent commented Sep 25, 2024

@dalexeev I addressed your comment

@Mickeon Mickeon requested a review from dalexeev September 26, 2024 18:44
@pafuent pafuent force-pushed the inst_to_dict_nesting_support branch from 5acece9 to f1f96e9 Compare October 17, 2024 13:28
@pafuent pafuent requested review from a team as code owners October 17, 2024 13:28
@pafuent
Copy link
Contributor Author

pafuent commented Oct 17, 2024

Friendly remainder

@pafuent pafuent force-pushed the inst_to_dict_nesting_support branch from f1f96e9 to 153b526 Compare November 1, 2024 20:32
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay. I left a few comments, but they don't need to be fixed. I have doubts about whether we should make inst_to_dict() and dict_to_inst() recursive. Perhaps we should deprecate these functions instead. Let me explain why.

  1. You implemented recursive representation only for object properties, but nested objects can also be array elements and dictionary keys/values.
  2. Arrays and dictionaries can be typed, so they also need to be represented in a special way (say, as {"@type": "Array", "@elem_builtin_type": "int", "@elements": [1, 2, 3]}). Because we can't, for example, put a dictionary representation of an object into an Array[Object].
  3. Arrays, dictionaries, and objects are passed by reference. Currently, inst_to_dict() creates a shallow copy, and all reference types in the dictionary representation are the same values ​​as in the original object (see example below). If we make a deep version of inst_to_dict(), it would probably have to create duplicates, because of p. 2. This would be inconsistent with the flat version, which is essentially a separate serialization function with different behavior, hidden in inst_to_dict() behind an optional parameter.
  4. We already have other serialization functions: var_to_bytes(), var_to_str(), and the new JSON functionality (see Ability to convert native engine types to JSON and back. #92656). I think 4 types of serialization is too many. var_to_str() has security concerns, but it seems like the new JSON methods are aimed at being more secure (at least they have appropriate parameters). I think we should focus on the core serialization methods and deprecate the GDScript specific inst_to_dict() and dict_to_inst() functions.
class Test:
    var a = []

func _ready():
    var test = Test.new()
    var dict = inst_to_dict(test)
    print(dict) # { "@subpath": ^"Test", "@path": "res://node.gd", &"a": [] }
    print(test.a) # []
    dict.a.append(1)
    print(dict) # { "@subpath": ^"Test", "@path": "res://node.gd", &"a": [1] }
    print(test.a) # [1]

Comment on lines 279 to 290
DEBUG_VALIDATE_ARG_COUNT(1, 1);
DEBUG_VALIDATE_ARG_TYPE(0, Variant::DICTIONARY);
static inline void inst_to_dict(Variant *r_ret, const Variant **p_args, int p_arg_count, Callable::CallError &r_error) {
DEBUG_VALIDATE_ARG_COUNT(1, 2);

Copy link
Member

Choose a reason for hiding this comment

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

Looks like you accidentally deleted DEBUG_VALIDATE_ARG_TYPE(0, Variant::OBJECT); and DEBUG_VALIDATE_ARG_TYPE(0, Variant::DICTIONARY); when rebasing the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those two checks are now made on the recursive versions _inst_to_dict and _dict_to_inst respectively

@pafuent pafuent force-pushed the inst_to_dict_nesting_support branch from 153b526 to 3f60bbe Compare November 5, 2024 02:34
@Ivorforce
Copy link
Member

Ivorforce commented Nov 5, 2024

I have doubts about whether we should make inst_to_dict() and dict_to_inst() recursive. Perhaps we should deprecate these functions instead.

I don't disagree. I'd like to know what a typical use-case for these functions is (or at least what they are currently being used for). Perhaps @pafuent can comment on that? I do occasionally use python's vars(), though that creates a shallow dictionary.

@pafuent
Copy link
Contributor Author

pafuent commented Nov 5, 2024

To be honest the only use case that I know is to serialize/unserialize data

Fixes godotengine#6533

Making GDScript inst_to_dict/dict_to_inst utility functions recursive.
Adding also a new macro to validate the number of the required arguments
and another to validate that an argument is boolean.
@pafuent
Copy link
Contributor Author

pafuent commented Nov 29, 2024

Thanks for the reviews and explaining thoroughly why this methods should be deprecated!!! (I really appreciate it)
Closing it because its superseded by #99121

@pafuent pafuent closed this Nov 29, 2024
@pafuent pafuent deleted the inst_to_dict_nesting_support branch November 29, 2024 10:16
@dalexeev dalexeev removed this from the 4.x milestone Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inst2dict does not turn classes inside classes to dict
5 participants