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

Implement approx_eq_eps, to_untyped and from_untyped in more places #366

Merged
merged 3 commits into from Jul 23, 2019

Conversation

@nical
Copy link
Collaborator

nical commented Jul 22, 2019

Fixes #362 and addresses a comment of https://phabricator.services.mozilla.com/D38455


This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented Jul 22, 2019

r? anyone


/// Drop the units, preserving only the numeric value.
#[inline]
pub fn to_untyped(&self) -> RigidTransform3D<T, UnknownUnit, UnknownUnit> {

This comment has been minimized.

@kvark

kvark Jul 23, 2019

Member

would this be equivalent to multiplying by Scale<f32, T, UnknownUnit>? If so, there might be a simpler and more universal way to get to/from untyped

This comment has been minimized.

@nical

nical Jul 23, 2019

Author Collaborator

I'm note sure I follow, rigid transforms don't have a scale. to/from_untyped is mostly there because you have your properly typed code and sometimes you delegate some work to a library that doesn't use units or doesn't use your units, so you cast the unit away for API compatibility.

I wonder if what you mean by scale is some sort of typed identity transform to reify the notion of casting between units in a more explicit way. Maybe it'd make sense in some context? I think that in the case of casting to the unknown unit all we want is to just give up some type info for API compat reason between some independent pieces of code and to/from_untyped are the simplest and most natural ways in that situation.

This comment has been minimized.

@kvark

kvark Jul 23, 2019

Member

You are correct, rigid transforms don't have a scale, so Scale is inapplicable.


/// Returns true is this transform is approximately equal to the other one, using
/// a provided epsilon value.
pub fn approx_eq_eps(&self, other: &Self, eps: T) -> bool {

This comment has been minimized.

@kvark

kvark Jul 23, 2019

Member

the matrix API receives epsilon by reference. Is there a strong reason to break consistency at this level?

This comment has been minimized.

@nical

nical Jul 23, 2019

Author Collaborator

Sorry that was me hesitating and forgetting to clean it up. I'd rather we consistently pass it by value but I don't care enough to change it.

@@ -335,6 +337,20 @@ where T: Copy + Clone +
self.m43.approx_eq(&other.m43) && self.m44.approx_eq(&other.m44)
}

/// Returns true is this transform is approximately equal to the other one, using
/// a provided epsilon value.
pub fn approx_eq_eps(&self, other: &Self, eps: T) -> bool

This comment has been minimized.

@kvark

kvark Jul 23, 2019

Member

I do wonder if such epsilon comparison is useful at this level. Do you have a use case in mind?
A small change in one of the components may contributing to arbitrary differences in the transformed vectors, so it's not like we can reason about the locality of error here.

This comment has been minimized.

@nical

nical Jul 23, 2019

Author Collaborator

Well, this one we already have a use case for in WebRender with on of Glenn's recent patches. I don't think the change is completely arbitrary. It's as arbitrary as any similar chain of additions and multiplications where a small increment somewhere could be magnified by a factor coming from another term, but very very tiny epsilon still lead to tiny changes in practice unless you apply a huge scale or translation.

To extend my thinking a bit, I believe that what doesn't make sense is to have approx_eq without explicitly passing the epsilon because the threshold that you need always depends on what arithmetic you do beforehand and a bit of knowledge about reasonable input ranges for your usecase.

This comment has been minimized.

@kvark

kvark Jul 23, 2019

Member

It's not just about huge scaling. This is a 3D transform that is subject to perspective. Any changes to W component bring a huge deal of non-linearity to this.

@kvark
kvark approved these changes Jul 23, 2019
Copy link
Member

kvark left a comment

Have a weak yes here. I don't like the approx comparison on perspective matrices... but if @gw3583 is already using those, then at least it's a practical concern.

@nical nical force-pushed the nical:mat-approx-eq branch from 648ea1f to 0b9b518 Jul 23, 2019
@nical
Copy link
Collaborator Author

nical commented Jul 23, 2019

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2019

📌 Commit 0b9b518 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2019

Testing commit 0b9b518 with merge 582df22...

bors-servo added a commit that referenced this pull request Jul 23, 2019
Implement approx_eq_eps, to_untyped and from_untyped in more places

Fixes #362 and addresses a comment of https://phabricator.services.mozilla.com/D38455

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/366)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2019

☀️ Test successful - checks-travis
Approved by: kvark
Pushing 582df22 to master...

@bors-servo bors-servo merged commit 0b9b518 into servo:master Jul 23, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
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.

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