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

Draft+Arch: Fix crash related to SoBrepEdgeSet #7528

Conversation

Roy-043
Copy link
Contributor

@Roy-043 Roy-043 commented Sep 26, 2022

Forum topic:
https://forum.freecadweb.org/viewtopic.php?f=23&t=72122

  • Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
  • In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
  • Your branch is rebased on latest master git pull --rebase upstream master
  • All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
  • All commit messages are well-written ex: Fixes typo in Draft Move command text
  • Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
  • Commit messages include issue #<id> or fixes #<id> where <id> is the issue ID number from our Issues database in case a particular commit solves or is related to an existing issue. Ex: Draft: fix typos - fixes #4805

@github-actions github-actions bot added the WB Draft Related to the Draft Workbench label Sep 26, 2022
@luzpaz luzpaz added the Crash For issues describing crashes or PRs fixing one label Sep 26, 2022
@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7528. Pipeline 650877208 was triggered at efd8f11. All CI branches and pipelines.

@yorikvanhavre
Copy link
Member

I think this will have the effect of making the dimension lines not selectable anymore...
Maybe an import FreeCADGui somewhere before using SoBrePEdgeSet would work too? I'll test

@Roy-043
Copy link
Contributor Author

Roy-043 commented Sep 27, 2022

Ooops. Thanks for testing.

Looking at this:
https://forum.freecadweb.org/viewtopic.php?p=147107#p147107
Maybe the solution is to use import PartGui?

@Roy-043
Copy link
Contributor Author

Roy-043 commented Sep 27, 2022

import PartGui + "SoBrePEdgeSet" works.

There may be another way to solve this though.
coin.SoType.fromName("SoBrepEdgeSet").createInstance()
also creates a SoIndexedLineSet, but with some different properties apparently?

@marioalexis84
Copy link
Member

marioalexis84 commented Sep 27, 2022

Import PartGui is the solution because SoBrepEdgeSet is a custom FreeCAD Coin3D node initialized when PartGui is loaded.

coin.SoType.fromName("SoBrepEdgeSet").createInstance()
also creates a SoIndexedLineSet, but with some different properties apparently?

That's because SoBrepEdgetSet inherits from the Coin3d node SoIndexedLineSet

@Roy-043
Copy link
Contributor Author

Roy-043 commented Sep 27, 2022

@marioalexis84: Thanks for explaining. I'll revise my PR then.

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7528. Pipeline 652004482 was triggered at 951d14d. All CI branches and pipelines.

@marioalexis84
Copy link
Member

marioalexis84 commented Sep 27, 2022

There are still two problems related to the use of SoBrepEdgeSet in the Arch workbench:
A crash with ArchBuildingPart and a raised AttributeError with AchSectionPlane.

The reason for this behavior is that the Draft objects edited in this commit and the previously named Arch objects are App::FeaturePython (or App::GeometryPython), but not objects from Part workbench like Part::FeaturePython.
Therefore, at object creation time when the document is loaded, the PartGui module is not imported and the SoBrepEdgeSet data is not initialized.
The crash is generated because coin.SoType.fromName("SoCustomType").createInstance() returns None if the data for SoCustomType was not previously initialized.
Then, trying to add None using the addChild method of SoGroup Coin objects leads to a crash, while trying to access attributes leads to an AttributeError.

@Roy-043 Roy-043 changed the title Draft: Fix crash when opening file with only dimensions Draft+Arch: Fix crash relatef to SoBrepEdgeSet Sep 28, 2022
@Roy-043 Roy-043 changed the title Draft+Arch: Fix crash relatef to SoBrepEdgeSet Draft+Arch: Fix crash related to SoBrepEdgeSet Sep 28, 2022
@Roy-043
Copy link
Contributor Author

Roy-043 commented Sep 28, 2022

@marioalexis84 Thanks again! I have included the 2 Arch files you refer to.

I have placed the import PartGui line close to the coin.SoType.fromName("SoBrepEdgeSet").createInstance() line for clarity.

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7528. Pipeline 652811697 was triggered at b771004. All CI branches and pipelines.

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7528. Pipeline 652819713 was triggered at 853f2a9. All CI branches and pipelines.

@marioalexis84
Copy link
Member

I'm not a big fan of importing modules inside functions, but I think @yorikvanhavre has no problem with that.
Can you squash the commits into one so don't pollute the branch with unnecessary commits?

@Roy-043 Roy-043 force-pushed the Draft-Fix-crash-when-opening-file-with-only-dimensions branch from 853f2a9 to 093368a Compare September 28, 2022 15:33
@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7528. Pipeline 653165172 was triggered at 093368a. All CI branches and pipelines.

@luzpaz luzpaz added the WB Arch Related to the Arch Workbench label Oct 3, 2022
@yorikvanhavre
Copy link
Member

I agree that doing imports in the middle of functions is pretty much anti-pythonic... But at the moment it's still (for me at least) the easiest and "clearest" way (meaning: easiest to locate and change in the future) to avoid loading heavy modules like Part when it's not needed. I consider this mostly as a temporary workaround.

I know there is a delayed loading module, but it seems to me to complicate the code even more...

Let's merge this as we have identified and nailed the issue and "it works", and the in-function imports are another matter to solve some day...

@yorikvanhavre yorikvanhavre merged commit 481f5f6 into FreeCAD:master Oct 7, 2022
@Roy-043 Roy-043 deleted the Draft-Fix-crash-when-opening-file-with-only-dimensions branch October 7, 2022 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Crash For issues describing crashes or PRs fixing one WB Arch Related to the Arch Workbench WB Draft Related to the Draft Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants