-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
add clip plane changes #230
Conversation
…l-animated/agave into feature/add-clip-plane # Conflicts: # renderlib/io/CMakeLists.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some non-blocking comments and questions!
ClipPlaneTool(Plane plane, glm::vec3& pos) | ||
: ManipulationTool(0) | ||
, m_plane(plane) | ||
, m_pos(pos) | ||
{ | ||
// assumes pos is in plane! if not, there will be trouble. | ||
// assert(glm::dot(m_plane.normal, m_pos) == m_plane.d); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to like, project m_pos
onto the provided plane?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the pos data stored by the clip plane tool doesn't have to precisely match the m_plane anymore. At init, I forget why I commented this out (and it may just be because I needed an epsilon). I'll have to put the code back and see why.
uses: ilammy/msvc-dev-cmd@v1 | ||
- name: windows install deps | ||
if: matrix.os == 'windows-latest' | ||
if: matrix.os == 'windows-2019' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these changes related to the broken build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
@@ -111,7 +111,7 @@ jobs: | |||
cpack -D CPACK_PACKAGE_FILE_NAME=${{ matrix.artifact }} | |||
shell: cmd | |||
- name: Upload windows artifact | |||
if: matrix.os == 'windows-latest' | |||
if: matrix.os == 'windows-2019' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not blocking but it seem like you could make this a variable you can reference instead of having to change this so many places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is noise you can ignore because it should stabilize once tensorstore is settled again. It's just me thrashing on it. This should not be changing this often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a few comments
Resolves #228
Add user clipping plane functionality.
Also adds some basic UI for enabling, showing/hiding, and manipulating the clip plane but the UI will be more or less completely redone. See #233
Note: gh-actions build is broken on windows but due to dependencies and not this code change.