-
Notifications
You must be signed in to change notification settings - Fork 9
[FXC-3495] [SCFD-4987] Mirroring interface, coordinate system and draft context manager. #1618
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
base: main
Are you sure you want to change the base?
Conversation
|
@codex review the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
flow360/component/project.py
Outdated
| def _load_mirror_status_from_asset(asset: AssetBase) -> Optional[MirrorStatus]: | ||
| """Get the mirror status from the asset""" | ||
|
|
||
| mirror_status_dict = ( | ||
| # pylint: disable=protected-access | ||
| AssetBase._get_simulation_json(asset=asset, clean_front_end_keys=True) | ||
| .get("private_attribute_asset_cache", {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard mirror-status fetch for local drafts
Creating a draft from a locally instantiated asset (id is None, e.g., surface/volume meshes loaded from disk) now fails before entering the context because _load_mirror_status_from_asset unconditionally calls AssetBase._get_simulation_json, which requires _webapi to be present. Local assets never set _webapi, so this path raises an AttributeError instead of returning a DraftContext, breaking with create_draft(new_run_from=SurfaceMeshV2(id=None)) and the new mock_surface_mesh test fixture. Please skip the mirror-status fetch or short-circuit when the asset has no web API connection.
Useful? React with 👍 / 👎.
|
@codex review again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if entity_info is None: | ||
| raise Flow360RuntimeError( | ||
| "[Internal] DraftContext requires `entity_info` to initialize." | ||
| ) | ||
| self._token: Optional[Token] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow DraftContext to set _token
The class defines __slots__ without _token, but the constructor assigns self._token a few lines below (line 155). Because slotted classes disallow attributes not listed in __slots__, constructing a DraftContext (e.g., via create_draft(...)) will immediately raise AttributeError: 'DraftContext' object has no attribute '_token', so none of the draft or mirroring logic can run.
Useful? React with 👍 / 👎.
* 1. Added CS model * 2. Added inheritance support of CDSs * Refactor draft coordinate systems into dedicated manager * Add coordinate system status serialization and manager integration
Remaining stuff to be done:
[MIRROR]:
[COORDINATE SYSTEM]: