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 Normalized method to Quaternion.hh #169

Merged
merged 3 commits into from
Oct 26, 2020

Conversation

pxalcantara
Copy link
Contributor

Add the method Normalized to the Quaternion class.

This PR is related to #34

Signed-off-by: pxalcantara <[email protected]>
Signed-off-by: pxalcantara <[email protected]>
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Oct 20, 2020
@pxalcantara
Copy link
Contributor Author

@chapulina @j-rivero Could you help me with this CI error for Windows? I couldn't get the cause 🤔

By the way, Why the unit tests case for Quaternion.hh is so big? Shouldn't we split it into several test cases for each method, in order to be easier to detect the error?

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the unit tests case for Quaternion.hh is so big? Shouldn't we split it into several test cases for each method, in order to be easier to detect the error?

I think that's probably just old. I agree that having smaller tests would make it easier to handle regressions whenever we have any.

src/Quaternion_TEST.cc Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor

I couldn't tell what was wrong with the Windows build, so I retriggered it

@j-rivero
Copy link
Contributor

I couldn't tell what was wrong with the Windows build, so I retriggered it

I see an "internal compiler error". Usually means one of two things:

  • There is not enough RAM to compile the code
  • The code change triggered a bug in the compiler

We have run 869 and 870 on the same Windows node with this problem. I've changed the configuration to run on the other Windows node, let's see if it fails too: Build Status

@pxalcantara
Copy link
Contributor Author

Why the unit tests case for Quaternion.hh is so big? Shouldn't we split it into several test cases for each method, in order to be easier to detect the error?

I think that's probably just old. I agree that having smaller tests would make it easier to handle regressions whenever we have any.

Can I make these changes? Should I open an issue for that?

@chapulina
Copy link
Contributor

Can I make these changes?

Yeah go for it! Thanks!

@pxalcantara
Copy link
Contributor Author

Can I make these changes?

Yeah go for it! Thanks!

Just make PR but don't need to open a new issue right?

@chapulina
Copy link
Contributor

Just make PR but don't need to open a new issue right?

Yeah I think that's fine 🆗

@chapulina chapulina merged commit 680753a into gazebosim:ign-math6 Oct 26, 2020
@pxalcantara pxalcantara mentioned this pull request Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants