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.

Sign up for GitHub

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

Jump to bottom

Drawing dimensions only mode #529

Closed
wants to merge 1 commit into from
Closed

Drawing dimensions only mode #529

wants to merge 1 commit into from

Conversation

77maxikov
Copy link
Contributor

Found no use of constraint signs on printouts sometimes - added dimension only view mode for constraints

@phkahler phkahler requested a review from rpavlik April 24, 2020 22:05
rpavlik
rpavlik requested changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@rpavlik rpavlik left a comment

Choose a reason for hiding this comment

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

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.

src/ui.h
@@ -691,7 +689,6 @@ class GraphicsWindow {
void RemoveConstraintsForPointBeingDeleted(hEntity hpt);
void FixConstraintsForRequestBeingDeleted(hRequest hr);
void FixConstraintsForPointBeingDeleted(hEntity hpt);
void EditConstraint(hConstraint constraint);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this got removed.. I'm hitting a build issue related to it. Perhaps this was an accidental change?

src/ui.h
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this was removed - its uses were not removed resulting in build issues.

src/textwin.cpp
@@ -929,7 +991,7 @@ void TextWindow::Paint() {

canvas->SetLighting(lighting);
canvas->SetCamera(camera);
canvas->StartFrame();
canvas->NewFrame();
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

src/ui.h
// This sets what gets displayed.
bool showWorkplanes;
bool showNormals;
bool showPoints;
bool showConstruction;
bool showConstraints;
ShowConstraintMode showConstraints;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

rpavlik
rpavlik reviewed Apr 27, 2020
View reviewed changes
src/textwin.cpp
@@ -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 =
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not leave old code in comments. That's what git is for, just remove the commented-out part.

@rpavlik
Copy link
Contributor

rpavlik commented Apr 27, 2020

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:

File ../src/platform/platform.cpp, line 579, function LoadResource:
Assertion failed: ReadFile(ResourcePath(name), &cache[name]).
Message: Cannot read resource.

Presumably it can't find the new icons? Not familiar with how resources work in this build.

@whitequark
Copy link
Contributor

Not familiar with how resources work in this build.

They need to be added to res/CMakeLists.txt.

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.

@rpavlik
Copy link
Contributor

rpavlik commented Apr 28, 2020

@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.

@whitequark
Copy link
Contributor

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.

Ah, got it--thanks for cleaning it up!

@77maxikov
Copy link
Contributor Author

Fixed in https://github.com/77maxikov/solvespace/tree/dimonly-fix/ Should it be PRed?

@rpavlik
Copy link
Contributor

rpavlik commented Apr 29, 2020

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 --force-with-lease)

@phkahler phkahler force-pushed the master branch 2 times, most recently from dfb96de to 3f09eaf Compare November 7, 2020 20:39
@ruevs
Copy link
Member

ruevs commented Jan 13, 2023

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...

@77maxikov
Copy link
Contributor Author

Working on it

@ruevs
Copy link
Member

ruevs commented Jan 13, 2023

#1329

@ruevs
Copy link
Member

ruevs commented Jan 18, 2023

Merged in #1329

@ruevs ruevs closed this Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpavlik rpavlik rpavlik requested changes

Assignees
No one assigned
Labels
waiting-on-author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@77maxikov @rpavlik @whitequark @ruevs