Derive Ord and PartialOrd for ClassName

[mivort]

This PR adds two simple derives (Ord and PartialOrd) to allow the storage of ClassName type values in ordered maps (BTreeSet/BTreeMap etc.).

[Bromeon]

Thanks a lot!

I'm wondering, would the expectation here not be lexicographical ordering? Otherwise operations like sort or binary_search are both surprising and potentially different in different runs 🤔

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-928

[mivort]

Thanks a lot!

I'm wondering, would the expectation here not be lexicographical ordering? Otherwise operations like sort or binary_search are both surprising and potentially different in different runs 🤔

Not sure if this might be counter-intuitive, but implication here is about ID stability only within a single run, without lexicographical order. ClassName already provides the ability to be stored in hash maps, but there are cases when linear (O(n)) iteration might be preferable to lookup/insertion performance (compared to HashMap, which might be slower to iterate in some cases due to bucket traversal).

[Bromeon]

Not sure if this might be counter-intuitive, but implication here is about ID stability only within a single run, without lexicographical order.

What do you mean with "ID stability"? Ordering?

If yes, I'm aware that this is the implication -- my argument was that this isn't necessarily a good implication 😉 even more so if undocumented.

---

ClassName already provides the ability to be stored in hash maps, but there are cases when linear (O(n)) iteration might be preferable to lookup/insertion performance (compared to HashMap, which might be slower to iterate in some cases due to bucket traversal).

Could you elaborate the use case you encountered?

---

Btw, clippy error is fixed in #929, you can rebase after I merge that.

[Bromeon]

Maybe as prior art regarding the "non-lexicographical ordering dilemma", StringName suffers from the exact same problem. There, I added StringName::transient_ord() as a solution.

I'm not sure if this would make sense here, too -- arguably the expectations for strings to be lexicographic is still higher than for class names, although "name" may have some of that connotation, as well.

Either way, we should definitely provide the fast ordering check based on indices. But it should at the very least be documented (c.f. StringName docs), and maybe even require explicit opt-in like transient_ord(). Opinions on this?

[mivort]

Could you elaborate the use case you encountered?

I'm implementing a child node hook auto-registration mechanism, where parent node keeps BTreeMap<ClassName, Gd<Node> index. Having ClassName as a key allows to ensure that each node type gets added to the hooks only once. During a specific events, parent iterates over this map and calls methods on all nodes which previously added themselves to this index (this allows to avoid calls to all child nodes, so only ones which have registered previously will be pinged). Difference with Godot signals here is that 'hooks' can return values (so, child node might provide an instant feedback to its parent).

It's possible to use HashMap for this, but linear iteration seems to be more important in this specific case.

[Bromeon]

It's possible to use HashMap for this, but linear iteration seems to be more important in this specific case.

Just to be clear, you're not looking for one of the following properties, right?

• Both HashMap and BTreeMap allow iteration over all elements.
• In both cases, iteration order is not the insertion order.
• With the proposed Ord impl, the order is also non-deterministic in both cases (for BTreeMap, it depends on non-local context, such as when a class was registered -- you cannot determine the order by looking at the values themselves).

With "linear", do you mean BTreeMap's current memory layout, as documented here?

Currently, our implementation simply performs naive linear search. This provides excellent performance on small nodes of elements which are cheap to compare. However in the future we would like to further explore choosing the optimal search strategy based on the choice of B, and possibly other factors. Using linear search, searching for a random element is expected to take B * log(n) comparisons, which is generally worse than a BST. In practice, however, performance is excellent.

If yes, that's definitely a valid use case. Do you have any input on my API comment above?

[mivort]

Sorry for not being clear on what I've meant by "linear".

Both HashMap and BTreeMap allow iteration over all elements.
With "linear", do you mean BTreeMap's current memory layout, as documented here?

Yeah, that's what I've meant. HashMap iteration performance might depend not only on number of its elements, but also on its current capacity. BTreeMap current implementation stores its values in contiguous array, allowing for fast iteration.

Either way, we should definitely provide the fast ordering check based on indices. But it should at the very least be documented (c.f. StringName docs), and maybe even require explicit opt-in like transient_ord(). Opinions on this?

If yes, that's definitely a valid use case. Do you have any input on #928 (comment) above?

I agree that it might feel confusing, and type wrapper solution might be preferable, since my case is pretty a niche one. But I also wonder - may it be somewhat more logical to name ClassName type as ClassId (since it stores u16 under the hood, providing conversion to String via global map) to make its meaning a bit more clearer? I suppose, it got its name following StringName type in Godot, but since it seems to be more of gdext entity, and doesn't get generated from strings, wouldn't it be more closer to Rust's own TypeId?

StringName in Godot probably got its name because it gets produced from string literals mostly, but ClassName is produced in a way which is more similar to Rust's std::any::TypeId::of<...>.

[Bromeon]

I agree that it might feel confusing, and type wrapper solution might be preferable, since my case is pretty a niche one. But I also wonder - may it be somewhat more logical to name ClassName type as ClassId (since it stores u16 under the hood, providing conversion to String via global map) to make its meaning a bit more clearer? I suppose, it got its name following StringName type in Godot, but since it seems to be more of gdext entity, and doesn't get generated from strings, wouldn't it be more closer to Rust's own TypeId?

Hm, very good points 🤔

Historically, it used to be a simple &'static str containing the class name. Only recently it evolved into an indirect storage, driven by performance needs. So yes, maybe "ID" is clearer than "name", but I also would probably not want to expose the internal integer ID just yet. I'll think a bit more about this...

For this PR, you can go ahead with deriving Ord/PartialOrd, but can you add an # Ordering section in the docs, similar to the existing one with StringName? Just mention that the ordering is not lexicographic and can change between runs; and that for lexicographic order, one can convert to GString or String.

[Bromeon]

Thanks! Can you apply the suggestion and squash the commits? Then it should be ready to go 🙂

[Bromeon]

Thanks a lot for your contribution! 😊