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

[TD]Dimension Reference Error Detection and Correction Ph 1 [#8878] #8989

Merged
merged 3 commits into from Mar 23, 2023

Conversation

WandererFan
Copy link
Contributor

This PR implements the first phase of a solution for issue #8878.

This phase detects changes in referenced geometry and corrects the reference if an exact match is available.

Phase 2 will address partial matches.

Thank you for creating a pull request to contribute to FreeCAD! Place an "X" in between the brackets below to "check off" to confirm that you have satisfied the requirement, or ask for help in the FreeCAD forum if there is something you don't understand.

  • [x ] Your Pull Request meets the requirements outlined in section 5 of CONTRIBUTING.md for a Valid PR

Please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.


@github-actions github-actions bot added the WB TechDraw Related to the TechDraw Workbench label Mar 22, 2023
@freecadci
Copy link

pipeline status for feature branch PR_8989. Pipeline 814596118 was triggered at e35151c. All CI branches and pipelines.

@aapo-aapo
Copy link
Contributor

I have tested the code in different branch, and read the new code a bit. Looks good, and I definitely agree with the proposed algorithm! I haven't found any problems with the code, but I've only run the version in different branch and not this PR (although I guess they are supposed to be exactly the same). Some small comments:

  • Functions bool fixExactMatch(); and void handleNoExactMatch(); are already added in the header files, but their bodies don't exist yet in the cpp files. They should probably not be introduced yet, but in the phase 2 of the new feature
  • The GitHub code tool Lint complains that GeometryMatcher(DrawViewDimension* dim) { m_dimension = dim; } should be marked with keyword "explicit" to avoid accidental implicit conversions later, and I agree with that
  • Lint seems to have some other (mostly syntax) suggestions, some of which are reasonable but not very important, and some are just silly like asking for longer than one letter variables

@WandererFan
Copy link
Contributor Author

Thanks for the comments - much appreciated.

fixExactMatch is used (@ 1428 in LTNPAutoCorrect3, 1430 in the newest LTNPAutoCorrect4) - that is where we fix the reference when we fix identical geometry in the pile.
handleNoExactMatch is used (@ 1484 / 1486) - the implementation for phase 1 doesn't do much, but it does clear the error flag.

I added the explict keyword you indicated and fixed some of the lint/tidy warning (at least the ones I understood!).

@aapo-aapo
Copy link
Contributor

Excellent! I have now compiled this specific PR, and have confirmed that it works as expected. I have used it to load some old files that have known corrupted dimensions (due to TechDraw evolution and TNP), and somehow the results with these files are better than before (less wrong dimensions)! My main reason was to check against unexpected crashes with old files, and I know that old files should not have been improved with this PR, but somehow I got better results than I should have had. However, my pet peeve, which is radically changing the ISO count for a dimensioned drawing still does not completely work, but seems to work better (as expected). This is one of the difficult cases, where edges will be split into several pieces; and thus it's not even possible to find edge-to-edge correspondence for dimensions, just vertex-to-vertex mapping is possible.

I also noticed that there are some divisions by View->getScale() in the code and got suspicious about division-by-zero, but it seems that the getScale() function itself is protected against wrong scales:
if (!(result > 0.0)) { result = 1.0; }

So, it compiles correctly, works, and I cannot find any problems with the code, so I'd say go for it! :)

Copy link
Contributor

@aapo-aapo aapo-aapo left a comment

Choose a reason for hiding this comment

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

Compiles, code and algorithm looks good to me, no bugs found in testing

@freecadci
Copy link

pipeline status for feature branch PR_8989. Pipeline 815056724 was triggered at 1e450aa. All CI branches and pipelines.

@WandererFan WandererFan merged commit 85216bd into FreeCAD:master Mar 23, 2023
5 checks passed
@WandererFan WandererFan deleted the LTNPAutoCorrect4 branch March 23, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB TechDraw Related to the TechDraw Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants