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

Assembly: migrationScript2 and migrationScript4 improved and refactored #18100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jiri-Macha
Copy link
Contributor

Description of Defects in migrationScript2 and migrationScript4:

Several additional issues have been identified in the Assembly functions migrationScript4 and migrationScript2, beyond those fixed in patch 71f0104.

One common problem is that the Joint property Reference1 or Reference2 can be set to an invalid format. For example, user can manually cause this issue by assigning an incorrect reference (e.g., the Z-axis) to these properties via the FreeCAD GUI. When this happens, internal errors (exceptions and their corresponding Python call stack) are reported in the Report View. However, such information is not helpful for the user to fix their 3D model.

Issues with the Current Implementation (Using migrationScript4 as Reference):

  • Lack of Type Validation for Reference1 and Reference2:

    The script does not check whether joint.Reference1[1] or joint.Reference2[1] is a sequence. If this condition is not met, statements like:

    sub1 = joint.Reference1[1][0]

    will throw an exception, printing the Python call stack.

  • Lack of Length Validation for Sequences:

    The script does not ensure that the sequence length is sufficient for indexing. For example, joint.Reference1 and joint.Reference2 must each have at least two elements to allow:

    joint.Reference1[1][1]
    joint.Reference2[1][1]

    Without validation, such calls can result in exceptions, printing the Python call stack.

  • No Feedback on Problematic Joint:

    The user is not informed about which Assembly joint in the object hierarchy has an invalid Reference1 or Reference2. Depending on the size of the 3D model, identifying the defect can be time-consuming and frustrating without proper feedback.

This patch is focused on:

  • Reporting Problematic Joint Path:

    A new function has been added to retrieve the full path to the Assembly joint. The return value (a string) is used to report the issue.

  • Improved Exception Handling:

    Instead of adding more conditional checks (see above), exception handling is implemented. AttributeError, IndexError, and TypeError exceptions are caught. The exception message (excluding the call stack) is displayed at the end of a user-friendly warning.

  • User-Friendly Warnings in the Report View:

    A single-line warning is now shown in the Report View, containing the following details:

    • Source of Warning: 'Assembly joint'
    • Location in the 3D Model: e.g., 'Assembly_XY.object_XY.joint_XY'
    • Problematic Attribute: e.g., 'ReferenceXY'
    • Exception Message: e.g., 'list index out of range'
  • Refactoring and Optimization:

    The functions were refactored and optimized at the end of the conversion process.

Advantages:

  • Cleaner Report View: Internal errors (e.g., Python call stacks) are no longer logged in the Report View, as they are not useful for end users. Only the exception message is displayed.

  • Improved User Feedback: Users can now identify the problematic Assembly joint in the object hierarchy, saving significant time and effort.

  • Robust and Concise Code: The solution simplifies the code while increasing its robustness.

Description of Defects in migrationScript2 and migrationScript4:

Several additional issues have been identified in the Assembly
functions migrationScript4 and migrationScript2, beyond those fixed in
patch 71f0104.

One common problem is that the Joint property Reference1 or Reference2
can be set to an invalid format. For example, user can manually cause
this issue by assigning an incorrect reference (e.g., the Z-axis) to
these properties via the FreeCAD GUI.  When this happens, internal
errors (exceptions and their corresponding Python call stack) are
reported in the Report View.  However, such information is not helpful
for the user to fix their 3D model.

Issues with the Current Implementation (Using migrationScript4 as
Reference):

- Lack of Type Validation for Reference1 and Reference2:

  The script does not check whether joint.Reference1[1] or
  joint.Reference2[1] is a sequence. If this condition is not met,
  statements like:

    sub1 = joint.Reference1[1][0]

  will throw an exception, printing the Python call stack.

- Lack of Length Validation for Sequences:

  The script does not ensure that the sequence length is sufficient
  for indexing. For example, joint.Reference1 and joint.Reference2
  must each have at least two elements to allow:

    joint.Reference1[1][1]
    joint.Reference2[1][1]

  Without validation, such calls can result in exceptions, printing the
  Python call stack.

- No Feedback on Problematic Joint:

  The user is not informed about which Assembly joint in the object
  hierarchy has an invalid Reference1 or Reference2. Depending on the
  size of the 3D model, identifying the defect can be time-consuming
  and frustrating without proper feedback.

This patch is focused on:

- Reporting Problematic Joint Path:

  A new function has been added to retrieve the full path to the
  Assembly joint. The return value (a string) is used to report the
  issue.

- Improved Exception Handling:

  Instead of adding more conditional checks (see above), exception
  handling is implemented. AttributeError, IndexError, and TypeError
  exceptions are caught. The exception message (excluding the call
  stack) is displayed at the end of a user-friendly warning.

- User-Friendly Warnings in the Report View:

  A single-line warning is now shown in the Report View, containing
  the following details:

    - Source of Warning:              'Assembly joint'
    - Location in the 3D Model: e.g., 'Assembly_XY.object_XY.joint_XY'
    - Problematic Attribute: e.g.,    'ReferenceXY'
    - Exception Message: e.g.,        'list index out of range'

- Refactoring and Optimization:

  The functions were refactored and optimized at the end of the
  conversion process.

Advantages:

- Cleaner Report View:
  Internal errors (e.g., Python call stacks) are no longer logged in
  the Report View, as they are not useful for end users. Only the
  exception message is displayed.

- Improved User Feedback:
  Users can now identify the problematic Assembly joint in the object
  hierarchy, saving significant time and effort.

- Robust and Concise Code:
  The solution simplifies the code while increasing its robustness.
@PaddleStroke
Copy link
Contributor

Looks good to me. @Jiri-Macha I supposed you have tested the migration scripts with various type of old files (and new files as well) just to make sure?

@Jiri-Macha
Copy link
Contributor Author

@PaddleStroke The most complex FreeCAD model I have made so far is composed of approximately 23 files.
I used the Assembly4 workbench a few times and the new Assembly workbench around three times.
I started modeling it in FreeCAD 0.21, but over time, I was forced to use a development
version of FreeCAD. If FreeCAD is not patched, the 'Report View' gets filled with Python
call stack log records. With the patch applied, I now see only a few compact warning lines.
See the screenshot:
2024-11-26 192912

As a FreeCAD user, I would also appreciate having the document name displayed before the
path to the problematic joint. Perhaps later, I will create another patch for this.

So, to answer your question: yes, I tested it, but I didn't perform extensive
tests — it is beyond my capabilities.

I do not have many assemblies created in the new Assembly workbench because it is still too new.
If possible, let it remain in a 'grey zone' where users can test it for a while.

I didn't change the logic of the migration code; I only improved its behavior when there is a wrong
reference or object (exception thrown), so I think, it should be safe to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mod: Assembly Related to the Integrated Assembly Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exceptions in the migrationScript2 and migrationScript4 logged into 'Report View'
2 participants