-
Notifications
You must be signed in to change notification settings - Fork 511
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
Issue #123: implement turntable mouse navigation #144
Conversation
… config checkbox (disabled by default)
src/solvespace.cpp
Outdated
@@ -70,6 +70,8 @@ void SolveSpaceUI::Init() { | |||
drawBackFaces = CnfThawBool(true, "DrawBackFaces"); | |||
// Check that contours are closed and not self-intersecting | |||
checkClosedContour = CnfThawBool(true, "CheckClosedContour"); | |||
// Use turntable mouse navigation | |||
checkTurntableNav = CnfThawBool(false, "CheckTurntableNav"); |
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.
"check" in "checkClosedContour" doesn't mean "checkbox", it's a part of the option name
src/mouse.cpp
Outdated
if(SS.checkTurntableNav) { | ||
projRight = orig.projRight.RotatedAbout(Vector::From(0, 1, 0), -s*dx); | ||
projUp = orig.projUp.RotatedAbout(Vector::From(orig.projRight.x, orig.projRight.y, 0), s*dy); // lock the Z to vertical | ||
} else { |
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.
please use our coding style
…r coding styles
Ok so commit d11ba14 addresses the variable naming and the coding style (I think). I think it was the one line being more than 100 characters long, but you didn't specify. Not sure if you will like the coding style yet. :) |
@dynamodan There are more tabs in that commit... Anyway, other than style issues I like it. Unfortunately we still have to sort out a CLA for external contributions so I can't merge it just yet, but I'm on it. |
Hm, don't you think the Y axis is not the right one? The initial sketch is on the XY plane, so if you're extruding it then the 'turntable' will not rotate parallel the extrusion vector. |
The build failed due to some write permission in the mac part of the travis CI: Doesn't have anything to do with this pull. |
|
@dynamodan, just use the z axis for upvector like all the normal guys. |
…ation, instead of Y.
@Evil-Spirit I tried to make Z (the blue axis) be vertical, but it causes some strange bugs. By default, Y is vertical, I'm sure of it, because of the way the horizontal and vertical constraints are messed up in some work planes when Z is "up". So... Y is up for now. |
@dynamodan Horizontal and vertical constraints have nothing to do with XYZ (i.e. the global coordinate system). They are defined in reference to the workplane they're in. |
@whitequark, I can try to fix this for Z axis |
Yeah, that only affects the text orientation. |
Well, not just the H and V lying on their sides, but the H is on the vertical segment, and the V is on the horizontal segment. It really confused me the other day, LOL! Anyway, in the context of turntable mouse navigation, I'm happy. |
@dynamodan Actually, now that I look closer, there's no issue. Try pressing F2 (without any turntable mode enabled). You'll see that horizontal will become what you expect from the constraints. |
I've discovered a new bug in my turntable nav implementation. When the view is aligned to a workplane that tilts the locked axis to horizontal (i.e. parallel to my desk surface), the view becomes locked to middlebutton+drag mouse navigation. (Other movements still work, such as ctrl+middlebutton+drag and shift+middlebutton+drag.) So in regard to this pull request, don't pull and merge just yet. |
This is done now -- I've fixed the issue of Z not staying vertical when some workplanes are active. Also, over the last few weeks I've been periodically pulling and merging from the master branch and testing to keep my fork fresh so whenever you're ready it should be good to pull. |
@dynamodan OK. I'll take a look soon. We unfortunately still don't have a CLA so I can't merge yet. |
We have a CLA now so hopefully this will be merged soon! |
That's good news! Did you do the merge yet? I just tonight merged from the latest (16540b1 at this moment) commit and successfully built in my fork. I try to do this periodically. |
Still waiting on CLA stuff... |
Thank you for your contribution, and welcome. The SolveSpace project has a Contributor License Agreement; in order for me to be able to merge your pull request, please sign it at https://cla-assistant.io/solvespace/solvespace. It will take only a small amount of your time. |
371f33b
to
fb1065d
Compare
I signed the CLA. I'll pull the latest and get my branch up to date, seems like that will make it easier to pull the turntable stuff. |
src/confscreen.cpp
Outdated
); | ||
} | ||
|
||
InvalidateGraphics(); // not sure if this is needed after the AnimateOnto, but hey |
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.
Nope, AnimateOnto implies InvalidateGraphics.
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.
It looks like you have some extra changes in extlib/libdxfrw
, can you exclude these? Also, it would be great if you could squash and rebase your commits so that it's just a single commit on top of master
, but if you can't figure that out, I can do it myself as well.
I'll try to see what changed in extlib/libdxfrw, but they weren't my changes that I know of. I believe I had simply run
as directed in the build instructions. Something could have changed in that submodule? |
I excluded extlib/libdxfrw. Not sure if I did it correctly, as now the appveyor and travis builds fail. |
No, you just deleted the submodule... |
I guess I don't understand how to do this. When I checkout your branch, the extlib/libdxfrw folder is empty. When I checkout my branch, the extlib/libdxfrw folder is empty. Same, same! But when I do |
Thanks! I'll take a closer look soon and merge--there isn't obviously wrong that I can see. |
Merged in master, thank you for your contribution and patience. |
I took a little time to implement this. It wasn't very difficult. This comes with config checkbox (disabled by default).