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

add PartialOrd support to all structs #252

Closed
wants to merge 1 commit into from

Conversation

@TyOverby
Copy link
Contributor

TyOverby commented Dec 17, 2017

Lack of PartialOrd support was my only issue when porting some of my code over to Euclid.

It's especially useful to preserve determinism after producing shapes in a non-deterministic manner.
Example:

  • Create a bunch shapes on the GPU (deterministically created; non-deterministic ordered)
  • Sort them on the CPU using this PartialOrd implementation
  • Now your collection of shapes is deterministically ordered!

Not the distinct lack of tests. I wanted to run this by folks here before spending too much time on writing tests.


This change is Reviewable

@SimonSapin
Copy link
Member

SimonSapin commented Dec 17, 2017

This ordering is arbitrary and has no meaningful relationship to geometry. If all you need is determinism in the iteration order of a set that’s fine, but it seems wrong to me to implement “the” ordering trait for this. Other trait impls like Add, where they exist, are implemented to make geometrical sense.

Perhaps instead of sort you could use sort_by_key with some arbitrary deterministic key, perhaps based on Hash?

@kvark
Copy link
Member

kvark commented Dec 18, 2017

Looks like a duplicate of #234, where the disposition appears to be: reject (edit: actually, "wait").
TL;DR: it doesn't make math sense, and it's relatively easy to implement for your wrapper types (in the way you mean it! since there is no one way to do it).

@TyOverby TyOverby closed this Dec 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.