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

Updated examples to glm #6389

Merged
merged 17 commits into from
Oct 31, 2019
Merged

Conversation

roymacdonald
Copy link
Contributor

I made a big mistake with git and the easiest way to fix it was to get simply copy all the modified examples over the master and commit and PR again.
@bakercp

Copy link
Member

@bakercp bakercp left a comment

Choose a reason for hiding this comment

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

Review complete. Changes submitted via PR.

@roymacdonald
Copy link
Contributor Author

seems to be ok then to merge. the CI is passing. @arturoc does the CI compile the examples?
I will double check visually each example though. @bakercp did you visually check the iOS examples?
I'll try yo get android running again to visually check too.

Copy link
Contributor Author

@roymacdonald roymacdonald left a comment

Choose a reason for hiding this comment

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

ok

examples/3d/orientationExample/src/ofApp.cpp Outdated Show resolved Hide resolved
@bakercp
Copy link
Member

bakercp commented Oct 15, 2019

My visual inspection of the iOS examples was successful. As soon as we figure out the quaternion issue #6327 and confirm android, I think this is ready to go.

@roymacdonald
Copy link
Contributor Author

good, I hope we get the ofQuat issue solved soon. I will check androind over the next days and report back.

@ofTheo
Copy link
Member

ofTheo commented Oct 30, 2019

@bakercp is there anything else left? #6327 is closed now.

@bakercp
Copy link
Member

bakercp commented Oct 30, 2019

@ofTheo @roymacdonald Roy -- did you make the glm::rotation changes? I can't seem to find them in this PR.

@roymacdonald
Copy link
Contributor Author

roymacdonald commented Oct 30, 2019

@bakercp Nope. This function we found that glm has it does not act in the same way as the ofQuaternion. I was testing it while in Denver, I'll look at it again tomorrow. But i think that it is fine to merge all this as-is, and then I make a different PR with the fixed orientation example.

@ofTheo
Copy link
Member

ofTheo commented Oct 30, 2019

@roymacdonald sounds good. Are you able to make the changes I mentioned above?
I think it would be good to have them made to your branch before we merge.

Basically avoid empty constructors that don't set initial values as that will give undefined results.

@roymacdonald
Copy link
Contributor Author

@ofTheo sorry but I am not following. which changes did you mention? Where are those empty constructors?

@ofTheo
Copy link
Member

ofTheo commented Oct 30, 2019

sorry @roymacdonald I think my review requesting changes wasn't public. see above! Thanks!

@ofTheo
Copy link
Member

ofTheo commented Oct 30, 2019

ps: @roymacdonald

I think glm::rotation is a 1:1 replacement for ofQuaternion::makeRotate

Here is a quick example that switches between the two.
It seems identical to me. Let me know if I am missing something :)

ofEasyCam cam;

//--------------------------------------------------------------
void ofApp::draw(){
    ofEnableLighting();
    ofLight l1;
    l1.setPosition(300, 300, 300);
    l1.enable();
    ofEnableDepthTest();
    
    cam.begin();
    
        ofQuaternion q;
        q.makeRotate(glm::vec3(0, 1, 0), glm::vec3(1, 0, 0));
        
        glm::quat qGLM = q;
        if( ofGetSeconds() % 2 == 0 ){
            glm::quat qGLM = glm::rotation(glm::vec3(0, 1, 0), glm::vec3(1, 0, 0));
        }
        
        ofMultMatrix( glm::toMat4(qGLM) );
        
        ofDrawCone(glm::vec3(0), 50, 300);
    cam.end();
}

@roymacdonald
Copy link
Contributor Author

@ofTheo @bakercp I've updated the orientation example as well as the glm constructor issue that theo mentioned. I don't know if there are more issues with this glm constructor thing.

@ofTheo
Copy link
Member

ofTheo commented Oct 31, 2019

Thanks @roymacdonald !
As far as I can tell this looks good to merge.

@roymacdonald
Copy link
Contributor Author

Super. Now on what does it depend to get it merged?

@arturoc arturoc merged commit b847305 into openframeworks:master Oct 31, 2019
@arturoc
Copy link
Member

arturoc commented Oct 31, 2019

thanks roy!

@bakercp
Copy link
Member

bakercp commented Oct 31, 2019

Epic. Thanks @roymacdonald !

@ofZach
Copy link
Contributor

ofZach commented Oct 31, 2019

👏👏 thx 👏👏

@roymacdonald roymacdonald deleted the examplesToGlm branch October 31, 2019 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants