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

Rotate tool should only show front facing rings #169

Merged
merged 7 commits into from
Apr 1, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions renderlib/RotateTool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ RotateTool::draw(SceneView& scene, Gesture& gesture)

gesture.graphics.addCommand(GL_LINES);

glm::vec4 discPlane(camFrame.vz, -glm::dot(camFrame.vz, axis.p));

Comment on lines +264 to +265
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you break down what this line is doing? I'm assuming it's making a normal vector pointing towards the camera (to be the clipping plane's normal vector) but I'm not quite following how it's getting there.

Is camFrame.vz the vector pointing out/away from the camera, and the dot product is getting a magnitude along that vector...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is standard geometry/linear algebra. The construct for a plane: a normal vector and a distance from the origin. The plane equation is Ax+By+Cz=D. camFrame.vz is the view direction which I am using as the normal to the plane. That provides the A,B,C. And the plane is positioned at (axis.p dot camFrame.vz) units away from the eye. That's D. axis.p is the world space center of the gizmo.

When you "dot" this plane vector with a point (x,y,z,1), you get a number that is positive or negative if the point is on either side of the plane. That's the equivalent to the value of Ax+By+Cz-D. if it's zero, the point is on the plane.

Copy link
Contributor

Choose a reason for hiding this comment

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

...And the plane is positioned at (axis.p dot camFrame.vz) units away from the eye...

Clarification question, should this be units away from the origin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, my mistake- it's in world space units from the origin, not from the eye.

float axisscale = scale * 0.85f;
// Draw the x ring in yz plane
{
Expand All @@ -269,7 +271,7 @@ RotateTool::draw(SceneView& scene, Gesture& gesture)
if (m_activeCode == RotateTool::kRotateX) {
color = glm::vec3(1, 1, 0);
}
gesture.drawCircle(axis.p, axis.l.vy * axisscale, axis.l.vz * axisscale, 48, color, 1, code);
gesture.drawCircle(axis.p, axis.l.vy * axisscale, axis.l.vz * axisscale, 48, color, 1, code, &discPlane);
}
// Draw the y ring in xz plane
{
Expand All @@ -278,7 +280,7 @@ RotateTool::draw(SceneView& scene, Gesture& gesture)
if (m_activeCode == RotateTool::kRotateY) {
color = glm::vec3(1, 1, 0);
}
gesture.drawCircle(axis.p, axis.l.vz * axisscale, axis.l.vx * axisscale, 48, color, 1, code);
gesture.drawCircle(axis.p, axis.l.vz * axisscale, axis.l.vx * axisscale, 48, color, 1, code, &discPlane);
}
// Draw the z ring in xy plane
{
Expand All @@ -287,7 +289,7 @@ RotateTool::draw(SceneView& scene, Gesture& gesture)
if (m_activeCode == RotateTool::kRotateZ) {
color = glm::vec3(1, 1, 0);
}
gesture.drawCircle(axis.p, axis.l.vx * axisscale, axis.l.vy * axisscale, 48, color, 1, code);
gesture.drawCircle(axis.p, axis.l.vx * axisscale, axis.l.vy * axisscale, 48, color, 1, code, &discPlane);
}

// Draw the camera-axis rotation manipulator as a circle always facing the view
Expand Down
14 changes: 11 additions & 3 deletions renderlib/gesture/gesture.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,8 @@ Gesture::drawCircle(glm::vec3 center,
uint32_t numSegments,
glm::vec3 color,
float opacity,
uint32_t code)
uint32_t code,
glm::vec4* clipPlane)
{
for (int i = 0; i < numSegments; ++i) {
float t0 = float(i) / float(numSegments);
Expand All @@ -628,8 +629,15 @@ Gesture::drawCircle(glm::vec3 center,
glm::vec3 p0 = center + xaxis * cosf(theta0) + yaxis * sinf(theta0);
glm::vec3 p1 = center + xaxis * cosf(theta1) + yaxis * sinf(theta1);

graphics.addLine(Gesture::Graphics::VertsCode(p0, color, opacity, code),
Gesture::Graphics::VertsCode(p1, color, opacity, code));
if (clipPlane) {
if (glm::dot(*clipPlane, glm::vec4(p0, 1.0)) > 0 && glm::dot(*clipPlane, glm::vec4(p1, 1.0)) > 0) {
graphics.addLine(Gesture::Graphics::VertsCode(p0, color, opacity, code),
Gesture::Graphics::VertsCode(p1, color, opacity, code));
}
Comment on lines +639 to +642
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope of this PR, but if you have a small number of segments or the segments are really large on screen, segments will be hidden if p0 is in front of the clip plane and p1 is not (or vice-versa). This might cause visual popping as the whole segment is suddenly rendered or not.

A nicer fix would be to move p0 or p1 to lie on the plane in that case, but it's more code and the bug is probably not visible in AGAVE. Is this current solution (discarding the whole line segment) the standard behavior when rendering with clipping planes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is admittedly crude but it works if the circles use small enough segments. I'll refine it as needed for future usage. Currently there's nothing else that needs to clip, yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also possible to use gpu clip planes and have this all done in the shader with good interpolation, but it would result in potentially splitting up the geometry into more draw calls. Currently the code can accumulate lots of line segments into a single draw call even if they are drawing many different things. And while things are still super cheap to draw right now, I didn't feel like routing all the data into the shader and splitting draw calls if they need different clip planes.

} else {
graphics.addLine(Gesture::Graphics::VertsCode(p0, color, opacity, code),
Gesture::Graphics::VertsCode(p1, color, opacity, code));
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion renderlib/gesture/gesture.h
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,8 @@ struct Gesture
uint32_t numSegments,
glm::vec3 color,
float opacity,
uint32_t code);
uint32_t code,
glm::vec4* clipPlane = nullptr);

// does not draw a flat base
void drawCone(glm::vec3 base,
Expand Down
Loading