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

Handle infinite length during normalizing a vector. #297

Merged
merged 2 commits into from Jul 9, 2018

Conversation

@BorisChiou
Copy link
Contributor

BorisChiou commented Jul 7, 2018

Rotate3d may use some extremely large value in the rotate axis vector, and
this may cause infinite length, so the normalized vector becomes a zero vector.
Here, we scale the vector first if its length is infinite, and then do the normalization.


This change is Reviewable

@BorisChiou
Copy link
Contributor Author

BorisChiou commented Jul 7, 2018

@nical could you please review this? Bug 1467277 needs this patch to generate a correct rotate transform matrix in some special cases.

@BorisChiou
Copy link
Contributor Author

BorisChiou commented Jul 7, 2018

I'm not sure the workflow if I'd like to update euclid and gecko, so I guess I need to create this PR, and then update servo/components/style/Cargo.toml, and then revender rust (or say bump the version?) in Gecko side.

Copy link
Collaborator

nical left a comment

Nice cacth! normalize is used in a lot of places and often enough we are confident that the range of values don't require the extra code, so I'd prefer to opt into the robustness here. Would you mind adding another method robust_normalize(self) -> T instead?

For updates I am not sure about the specifics of the style system, but since your PR doesn't introduce a breaking change, I think that all we need is to:

  • make changes to euclid on this repo
  • publish euclid on crates.io
  • update the Cargo.toml of that part that needs this change
  • in mozilla-central, go to toolkit/library/rust and run cargo update -p euclid
  • run the same cargo update command in toolkit/library/gtest/rust
  • run mach vendor rust at the root of mozilla-central.

When there is a breaking change we first need to go through all of the gecko crates that depend on euclid and update their version of euclid before we can do the steps above, which is a bit of a pain.

@BorisChiou
Copy link
Contributor Author

BorisChiou commented Jul 9, 2018

Cool! OK, let's add another method, robust_normalize(self) -> T. Thanks for the instructions.

@BorisChiou BorisChiou force-pushed the BorisChiou:master branch 2 times, most recently from 9a303ef to 58b40dc Jul 9, 2018
@BorisChiou BorisChiou force-pushed the BorisChiou:master branch from 58b40dc to 6406807 Jul 9, 2018
@BorisChiou
Copy link
Contributor Author

BorisChiou commented Jul 9, 2018

I updated this. @nical, couold you please check again? Thanks for the help.

@nical
Copy link
Collaborator

nical commented Jul 9, 2018

Neat! @bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jul 9, 2018

📌 Commit 6406807 has been approved by nical

@bors-servo
Copy link
Contributor

bors-servo commented Jul 9, 2018

Testing commit 6406807 with merge 4c254dc...

bors-servo added a commit that referenced this pull request Jul 9, 2018
Handle infinite length during normalizing a vector.

Rotate3d may use some extremely large value in the rotate axis vector, and
this may cause infinite length, so the normalized vector becomes a zero vector.
Here, we scale the vector first if its length is infinite, and then do the normalization.

<!-- 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/297)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 9, 2018

☀️ Test successful - status-travis
Approved by: nical
Pushing 4c254dc to master...

@bors-servo bors-servo merged commit 6406807 into servo:master Jul 9, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI 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.

None yet

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