Drawing dimensions only mode #529
Conversation
There was a problem hiding this comment.
It looks like your change is mixed with some unrelated (possibly accidental or accidental reversions? merge/rebase conflict resolution errors?) changes which are breaking the build, even after merging from master. Consider re-basing on master and auditing to make sure you've only included the changes you mean to here.
| @@ -691,7 +689,6 @@ class GraphicsWindow { | |||
| void RemoveConstraintsForPointBeingDeleted(hEntity hpt); | |||
| void FixConstraintsForRequestBeingDeleted(hRequest hr); | |||
| void FixConstraintsForPointBeingDeleted(hEntity hpt); | |||
| void EditConstraint(hConstraint constraint); | |||
There was a problem hiding this comment.
Not sure why this got removed.. I'm hitting a build issue related to it. Perhaps this was an accidental change?
| @@ -430,7 +429,6 @@ class TextWindow { | |||
| static void ScreenChangeShowContourAreas(int link, uint32_t v); | |||
| static void ScreenChangeCheckClosedContour(int link, uint32_t v); | |||
| static void ScreenChangeTurntableNav(int link, uint32_t v); | |||
| static void ScreenChangeImmediatelyEditDimension(int link, uint32_t v); | |||
There was a problem hiding this comment.
Not sure why this was removed - its uses were not removed resulting in build issues.
| @@ -929,7 +991,7 @@ void TextWindow::Paint() { | |||
|
|
|||
| canvas->SetLighting(lighting); | |||
| canvas->SetCamera(camera); | |||
| canvas->StartFrame(); | |||
| canvas->NewFrame(); | |||
There was a problem hiding this comment.
I don't understand what this change has to do with the others - is it perhaps an accident? I've now got:
../src/textwin.cpp:994:13: error: no member named 'NewFrame' in 'SolveSpace::ViewportCanvas'
canvas->NewFrame();
~~~~~~ ^
in clang 10 (after merging from master)
| // This sets what gets displayed. | ||
| bool showWorkplanes; | ||
| bool showNormals; | ||
| bool showPoints; | ||
| bool showConstruction; | ||
| bool showConstraints; | ||
| ShowConstraintMode showConstraints; |
There was a problem hiding this comment.
There are some references to showConstraints where it is still treated as a bool. Perhaps either leave this as a bool and add an extra variable for the display mode, or fix the other uses: including src/textscreens.cpp:171:8
| @@ -163,8 +219,19 @@ static ShowHideButton pointsButton = | |||
| { &(SS.GW.showPoints), "point", "points" }; | |||
| static ShowHideButton constructionButton = | |||
| { &(SS.GW.showConstruction), "construction", "construction entities" }; | |||
| static ShowHideButton constraintsButton = | |||
|
|
|||
| /*static ShowHideButton constraintsButton = | |||
There was a problem hiding this comment.
Let's not leave old code in comments. That's what git is for, just remove the commented-out part.
|
I rebased this on master, unstaged apparently unrelated changes, then added a commit to try to fix the other build errors (Not sure I have them entirely correct!) here: https://github.com/rpavlik/solvespace/tree/pr529 It does at least build, but crashes immediately with: Presumably it can't find the new icons? Not familiar with how resources work in this build. |
They need to be added to The concept behind this PR seems good to me, but I'm quite unhappy with the implementation (even after your cleanups). Feel free to resubmit a PR and request a review from me--I should have some free time to do this after 30th. |
|
@whitequark wasn't my idea, I just tried to get it building since I was suggested as a reviewer - hence why I pushed to a new branch. I agree the idea is good but the implementation seems like it touches a lot more stuff than I'd expect it too. |
Ah, got it--thanks for cleaning it up! |
|
Fixed in https://github.com/77maxikov/solvespace/tree/dimonly-fix/ Should it be PRed? |
|
It would be best to rebase your "fix" commit to squash it on to your other commits like I have done, and push it to this same branch again (with |
phkahler
force-pushed
the
master
branch
2 times, most recently
from
dfb96de to
3f09eaf
Compare
|
https://github.com/77maxikov/solvespace/tree/dimonly-fix looks like a cleaned up version of this. @77maxikov do you want to rebase it on master and open a new PR? This may be worth merging but last time I did not play with it since it would not even compile... |
|
Working on it |
|
Merged in #1329 |
Found no use of constraint signs on printouts sometimes - added dimension only view mode for constraints