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

Sketcher: BSpline DSH: implement OVP/widget #14079

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PaddleStroke
Copy link
Contributor

Fixes #12016
Fixes #12479

@AjinkyaDahale could you please give it a test and see if there is any problem?

@github-actions github-actions bot added the WB Sketcher Related to the Sketcher Workbench label May 17, 2024
@PaddleStroke PaddleStroke force-pushed the sk_bspline_ovp branch 2 times, most recently from 0b8317d to b7a54f1 Compare May 17, 2024 09:17
@AjinkyaDahale
Copy link
Contributor

Just a heads up: I'm having build issues right now (likely related to Manjaro update). I'll get back to you once it's all resolved.

@maxwxyz
Copy link
Collaborator

maxwxyz commented May 17, 2024

Nice!
@PaddleStroke do you think Polyline can get in for 1.0?

Copy link
Contributor

@AjinkyaDahale AjinkyaDahale left a comment

Choose a reason for hiding this comment

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

I'm still struggling to get this working on my device but here's some comment based just on looking at the code changes.

src/Mod/Sketcher/Gui/DrawSketchHandlerPolygon.h Outdated Show resolved Hide resolved
@AjinkyaDahale
Copy link
Contributor

AjinkyaDahale commented May 21, 2024

Some comments:

  1. I would suggest modes to only switch between periodic and non-periodic forms, and keeping any selected points. Currently, if one presses M, these points are automatically lost.
  2. If we enter a value in one of the distance/angle fields, and press tab, then even after putting in the point (with a left click), that value remains set. Additionally, I see that these numbers don't convert into constraints in the end, which is what I would expect to happen.
    Screencast from 2024-05-21 21-00-08.webm

@PaddleStroke
Copy link
Contributor Author

  1. I do not see the point of changing current situation. It feels that its only to make it possible to separate the code into 2 DSH files. Code should not dictate UI. This is a UI decision, I'd say it's for the @FreeCAD/design-working-group to weight in.
    It is true that changing the construction mode reset the tool, as it does with all the DSH. However indeed we could not reset it in this case. It would require a modification of the framework, but it would be needed for polyline anyway.

  2. Yes this is not normal. I need to fix this.

@AjinkyaDahale
Copy link
Contributor

I do not see the point of changing current situation. It feels that its only to make it possible to separate the code into 2 DSH files. Code should not dictate UI. This is a UI decision, I'd say it's for the @FreeCAD/design-working-group to weight in.

While I am interested in keeping the classes separate for the refactoring, my rationale for the M behaviour was that it keeps present points the same. If we can also easily toggle between periodic and nonperiodic, particularly through some other shortcut, that works. I'd imagine the mode switches for arc cannot reasonably preserve points as well.

It would require a modification of the framework, but it would be needed for polyline anyway.

Could you elaborate on the polyline case?

@PaddleStroke
Copy link
Contributor Author

I have fixed the 2 points you mentioned :
1 - Changing the construction mode does not reset the tool. You can toggle between controlPoint and Knot without loosing your bspline
2 - If you enter a value for the angle or the length, it is then reseted correctly.

@PaddleStroke
Copy link
Contributor Author

Could you elaborate on the polyline case?

The construction methodes of the polyline case will be Arc / line for the last segment. ie it's not a construction methode for the whole thing. So reseting will not be correct.

@FEA-eng
Copy link
Contributor

FEA-eng commented May 28, 2024

I wanted to test this but right after entering the second coordinate of the first point, I get a bunch of errors and in some cases FreeCAD even freezes:

15:45:47  Unhandled unknown exception caught in GUIApplication::notify.
15:45:48  Unhandled unknown C++ exception in ViewProvider::eventCallback (Event type: SoLocation2Event, object type: SketcherGui::ViewProviderSketch)

About FreeCAD info:

OS: Ubuntu 22.04.4 LTS (ubuntu:GNOME/ubuntu)
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.37545 (Git)
Build type: Unknown
Branch: sk_bspline_ovp
Hash: 3fae83bfe9425e4f78fc70cbe122177e3a2e55fd
Python 3.10.12, Qt 5.15.3, Coin 4.0.0, Vtk , OCC 7.5.1
Locale: English/United States (en_US)
Installed mods: 
  * OpenDark 2024.3.13

@PaddleStroke
Copy link
Contributor Author

You mean using the positioning OVP for the first point right?

@FEA-eng
Copy link
Contributor

FEA-eng commented May 29, 2024

You mean using the positioning OVP for the first point right?

Yes, I can enter the first coordinate but applying the second one results in those errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB Sketcher Related to the Sketcher Workbench
Projects
None yet
4 participants