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

[3.x] Support deep comparison of Array and Dictionary #42625

Merged
merged 2 commits into from
Nov 9, 2021

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Oct 7, 2020

This is a revival of #35816, but for 3.x. As such, it's just a safe subset of it; namely, recursive comparison is added to Dictionary and Array, but instead of changing how comparison currently works on them, it adds special deep_equal() functions, that one can use if desired.

And this PR indeed leverages deep_equal() in the following situations:

Some extra notes:

Last word:

I've written the following GDScript snippet to test how well deep_equal() works and also to contrast it with ==:

    # deep_equal() works as expected in every case

    # Array containing no other Arrays or Dictionaries (== works)
    var a1a = [1, 2, 3]
    var a1b = [1, 2, 3]
    assert(a1a == a1b)
    assert(deep_equal(a1a, a1b))

    # Array containing Array (== works)
    var a2a = [1, 2, 3, [1, 2, 3]]
    var a2b = [1, 2, 3, [1, 2, 3]]
    assert(a2a == a2b)
    assert(deep_equal(a2a, a2b))

    # Array containing Dictionary
    var a3a = [1, 2, 3, {"a": 1, "b": 2}]
    var a3b = [1, 2, 3, {"a": 1, "b": 2}]
    assert(a3a != a3b)
    assert(deep_equal(a3a, a3b))

    # Dictionary containing no other Dictionaries or Arrays (== doesn't work even in this case)
    var d1a = {"a": 1, "b": 2, "c": 3}
    var d1b = {"a": 1, "b": 2, "c": 3}
    assert(d1a != d1b)
    assert(deep_equal(d1a, d1b))

    # Dictionary containing Array
    var d2a = {"a": 1, "b": 2, "c": [1, 2, 3]}
    var d2b = {"a": 1, "b": 2, "c": [1, 2, 3]}
    assert(d2a != d2b)
    assert(deep_equal(d2a, d2b))

    # Dictionary containing Dictionary
    var d3a = {"a": 1, "b": 2, "c": {"e": 11, "f": 22}}
    var d3b = {"a": 1, "b": 2, "c": {"e": 11, "f": 22}}
    assert(d3a != d3b)
    assert(deep_equal(d3a, d3b))

Fixes #29221.


This PR is generously donated by IMVU.

@touilleMan
Copy link
Member

For 4.0 we are still in time to merge #35816 so that proper comparison is not a patch but an out-of-the-box feature.

I'm willing to rework the PR anytime needed to have it merge as soon as possible ;-)

I've added him as a co-author to it

😘

Also, @touilleMan, you'll noticed I've renamed recursive_equal to deep_equal. I hope that makes sense to you and you are willing to adopt that name in your PR for 4.0. 😊

I have the feeling deep_equal is meaningful given how the standard equal currently works (i.e. pointer equality vs contained data equality).
In 4.0 equal would just work as expected (always doing contained data equality) so calling the code responsible for the comparison recursive_equal makes more sens since it gives an important indication of how it works.

In the end I'm ok to change the name, but I'd like some additional opinion on the topic if possible before doing so ;-)

core/typedefs.h Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Oct 7, 2020

Nice. If this gets merged I can dump my compares from my GDScript function library.

recursive_equal makes more sense

That's more descriptive about it's implementation, rather than what it provides. I feel the adjectives aren't really needed. Additional implementation and parameter detail can be provided in it's doc entry.

For what its worth, the following signatures seem fine to me.
Array.matches(Array a, int depth = 0)
Array.equal(Array a, int depth = 0)
Array.equals(Array a, int depth = 0)
Array.is_equal(Array a, int depth = 0)

@RandomShaper RandomShaper force-pushed the fix_dict_3.2 branch 2 times, most recently from 44700d6 to 1156a08 Compare October 9, 2020 08:51
@RandomShaper RandomShaper requested review from a team as code owners March 12, 2021 12:26
Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
@akien-mga akien-mga modified the milestones: 3.2, 3.3 Mar 17, 2021
@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
@akien-mga akien-mga changed the title Support deep comparison of Array and Dictionary (3.2) [3.x] Support deep comparison of Array and Dictionary Mar 26, 2021
@RandomShaper RandomShaper modified the milestones: 3.4, 3.5 Oct 8, 2021
@RandomShaper
Copy link
Member Author

@touilleMan, I've rebased this. By now I have already lost track of the additional changes you've made to your PR for 4.0.

Could you please take a look and confirm this is not missing any important piece from yours?

RandomShaper and others added 2 commits November 9, 2021 15:08
...and expose it to GDScript.

Co-authored-by: Emmanuel Leblond <emmanuel.leblond@gmail.com>
@akien-mga akien-mga merged commit b0cd38b into godotengine:3.x Nov 9, 2021
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the fix_dict_3.2 branch November 9, 2021 15:38
@just-like-that
Copy link

just-like-that commented Feb 4, 2022

  • The new deep_equal() GDScript global function.

I was just wondering if deep_equals() with an "s" at the end has more meaning than deep_equal().
Equals has the meaning of "is equal". For this question I expect a boolean answer.
Maybe it's only me who expects equals() as I'm used to it from Java ;)

@RandomShaper
Copy link
Member Author

For the member functions (not exposed to scripting) I agree that deep_equals would sound better. About the global GDScript function, precisely because it's not a method, I see deep_equal as a noun, the name of an operator that is used with function call syntax, and as such I think the current name is fitting. Another way of defending the current name is that it's a verb in plural, like, "A and B equal to each other".

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.

5 participants