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 Primitives SceneGraph Examples #82

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

pomeroyb
Copy link
Contributor

@pomeroyb pomeroyb commented May 4, 2020

I've been meaning to get back into learning Magnum, and what better way than to learn than to help make some examples!

I saw that this was a desired sub project from mosra/magnum#102

I added a bit more functionality than the python example had, to control both the drawables and the camera. It's landed somewhere between the primitives example, then python scenegraph, and the picking example.

Let me know if any changes need to be made 👍

An example showing using primitives and the SceneGraph, as mentioned in: mosra/magnum#102

Implements DrawableObject subclass of Drawable3D, demonstrates parenting objects to the scene and to other objects, and shows local object transformations and camera transformations using left and right click respectively
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Thank you! Code looks great apart from a bunch of minor whitespace / tabs-vs-spaces issues which I can fix during merge, no problem. (Does your editor support .editorconfig, btw? Maybe there's a plugin at least. Then it would pick up indentation rules automatically.)

Can you show a screenshot so I can see how the example looks like without having to build it locally?

/* Dragging with left mouse button will rotate the objects locally */
if ((event.buttons() & MouseMoveEvent::Button::Left)) {
for (auto* o : _objects) {
(*o).transformLocal(Matrix4::rotationX(Rad{ delta.y() }) * Matrix4::rotationY(Rad{ delta.x() }));
Copy link
Owner

Choose a reason for hiding this comment

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

Would this be the same as

Suggested change
(*o).transformLocal(Matrix4::rotationX(Rad{ delta.y() }) * Matrix4::rotationY(Rad{ delta.x() }));
(*o).rotateYLocal(Rad{ delta.y() })
.rotateXLocal(Rad{ delta.x() }));

? It's a bit shorter to write.

explicit DrawableObject(Shaders::Phong& shader, const Color3& color, GL::Mesh& mesh, Object3D& parent, SceneGraph::DrawableGroup3D& drawables) : Object3D{ &parent }, SceneGraph::Drawable3D{ *this, &drawables }, _shader(shader), _color{ color }, _mesh(mesh) {}

void setColor(Color3 color) { _color = color; }
Color3 getColor() { return _color; }
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Color3 getColor() { return _color; }
Color3 color() const { return _color; }

Just for consistency -- all getters in Magnum are w/o the get prefix :)

Original authors — credit is appreciated but not required:

2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019 —
Vladimír Vondruš <mosra@centrum.cz>
Copy link
Owner

Choose a reason for hiding this comment

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

Feel free to list yourself here :)

@mosra mosra added this to the 2020.0a milestone May 7, 2020
@mosra mosra added this to TODO in Project management via automation May 7, 2020
@pomeroyb
Copy link
Contributor Author

pomeroyb commented May 9, 2020

Thanks for the feedback! I've made almost all the changes you suggested. The only one that needs a bit more review is the rotation. Using rotateLocal() does work fine, but the delta.x() and delta.y() need to be reversed to see the correct rotation. I've included a video showing what I mean:

https://youtu.be/6fQTFLv8ceQ

I'm not sure if this is a convention in Magnum or if I'm doing something wrong... 🤷‍♀️

@mosra
Copy link
Owner

mosra commented May 9, 2020

Hah, no, I clearly messed up in that suggestion, sorry. Will fix this during merge.

Thank you!

@mosra
Copy link
Owner

mosra commented May 12, 2020

In case you're wondering why no merge happened yet -- I wanted to take the Viewer tutorial and split the scenegraph-related stuff into a tutorial this example, as it's easier to absorb it in two smaller pieces. So I need to do some writing first :)

@pomeroyb
Copy link
Contributor Author

No worries at all! I'm writing examples to help myself learn Magnum in small steps -- being able to contribute them to the project is just a bonus. 👍

@mosra mosra modified the milestones: 2020.06, 2020.0b Jun 26, 2020
@mosra mosra mentioned this pull request Sep 26, 2020
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants