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

RID: Change comparison operators to use RID_Data id instead of address #59234

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Mar 17, 2022

This should helps making sorting more deterministic in physics and rendering.

The same change was done for 4.0 in 4f16397.

Alternative to #59192 fixing the issue as its root.
Helps with #25384 by making sorting deterministic - but I'm not sure it will make it predictable / as users expect it.

I could use help testing and making sure that this doesn't impact performance noticeably.


According to Sourcetrail, these operators are used like this:

  • RID::operator<:
    image
    Through Comparator this is also used in Map and Set
  • RID::operator<=: Used in Variant::evaluate
  • RID::operator>: Never used.

This should helps making sorting more deterministic in physics and rendering.

The same change was done for 4.0 in 4f16397.
@akien-mga akien-mga added this to the 3.5 milestone Mar 17, 2022
@akien-mga akien-mga requested a review from a team as a code owner March 17, 2022 10:11
@lawnjelly
Copy link
Member

Imo this is probably a slight improvement (as far as repeatibility when opening the same project). However anything using this method of sorting is pretty dodgy imo, as there is a certain amount of random chance involved here, depending on the historical order of RID allocation. It's just a very rough repeatable sort on the same run of the program.

There is an extra level of indirection here, but unless it proves to be a problem we can probably ignore it (we could always revert if necessary but I doubt it will be a problem).

Imo sorting on the RIDs might be okay for e.g. debugging logging but it is very questionable for anything gameplay wise, so we should be looking through with a fine toothed comb to see whether some better method of sorting could be employed.

Canvas items for instance to be "correct" as far as the user is concerned should probably be maintaining an ID within the tree traversal, although perhaps that has been ruled expensive to update each time one of them is changed. Maybe it can be done with a dirty flag or something.

@jordo
Copy link
Contributor

jordo commented Mar 17, 2022

This looks great. repeatable sort on the same run of the program is still valuable compared to totally random depending on what heap addresses your program receives each run, so still a net positive. FWIW this is exactly the type of issue that caused our physics simulation to go bananas for reproducibility when I first tried to use godot physics... (same inputs on subsequent runs on same binary & machine would result in different solver behaviour).

@lawnjelly
Copy link
Member

lawnjelly commented Dec 11, 2022

Just to note in #67273 this PR proves to cause problems when RID is used as a lookup in a Map where the RID may be dangling (i.e. still set to point to the RID object after it has been freed). Before, the data pointer could still be used for operator < comparisons, after this PR it now dereferences and causes a crash.

This is all the more reason to consider the points at which this has been used, and reconsider whether using RID as a key is the most sensible option, versus say an simple incrementing ID, or a handle (both of which may offer faster lookup).

N.B.

Also note there's a subtle difference between this PR (3.x) and the one for 4.x. The one for 4.x stores the id directly in the RID by value - this PR for 3.x does not, and requires dereferencing the object, and is thus subject to the above bug.

@akien-mga
Copy link
Member Author

Reverted with #69946.

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.

3 participants