Skip to content

coverage for all compas primitives #6

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.

Sign up for GitHub

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

Jump to bottom
Merged
tomvanmele merged 10 commits into from
Dec 5, 2025
Merged

coverage for all compas primitives #6

tomvanmele merged 10 commits into from
Dec 5, 2025

Conversation

@jf---
Copy link
Contributor

@jf--- jf--- commented Dec 3, 2025

Summary

  • removes problematic %pip install -q compas_notebook cell conflicting with dev mode (pip install -e .)
  • adds support for circle, ellipse, vector, plane, curve, surface primitives
  • adds analytical surface examples (spherical, cylindrical work without plugin)

Changes

  • removes pip install cell from all example notebooks (7 files)
  • adds 6 new scene objects: CircleObject, EllipseObject, VectorObject, PlaneObject, CurveObject, SurfaceObject
  • extends geometry converters with angular resolution for circles/ellipses
  • vector rendered as arrow with cone head
  • plane shown as grid
  • curve/surface discretization (requires nurbs plugin for creation)
  • updates geometry notebook with surface examples

jf--- added 4 commits November 19, 2025 12:33
@jf---
Adds support for rendering frames in the Notebook Viewer
1014b42 
Introduces functionality to visualize frames as 3D objects in the Notebook Viewer.

- Adds a `frame_to_threejs` conversion utility for transforming frames into PyThreeJS objects.
- Implements `ThreeFrameObject` for rendering frames in scenes, with axes visualized in red, green, and blue.
- Updates the registration process to include frame objects in the Notebook Viewer.
- Enhances the example notebook to demonstrate frame rendering capabilities.
- Fixes a type annotation issue in `line_to_threejs` and improves the `draw` method's return type annotations for consistency.

These changes enhance the visualization capabilities of the Notebook Viewer and improve type safety in the codebase.
@jf---
the initial cell:
ac86bb5 
`%pip install -q compas_notebook`
is problematic when working in dev mode and conflicts with `pip install -e .`
@jf---
add support for circle, ellipse, vector, plane, curve, surface primit…
192c10a 
…ives

- converters use angular resolution for circles/ellipses
- vector rendered as arrow with cone head
- plane shown as grid
- curve/surface discretization (requires nurbs plugin for creation)
@jf---
add surface examples using analytical surfaces
3aff457 
spherical and cylindrical surfaces work without plugin
@jf---
Copy link
Contributor Author

jf--- commented Dec 3, 2025

@tomvanmele this supercedes PR #4 let me close that one

tomvanmele
tomvanmele reviewed Dec 4, 2025
View reviewed changes
Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

generally looks good. just a few notes...

@tomvanmele tomvanmele self-assigned this Dec 4, 2025
@tomvanmele tomvanmele added the enhancement New feature or request label Dec 4, 2025
@tomvanmele
Copy link
Member

tomvanmele commented Dec 4, 2025

@jf--- the removal of the all block triggers the ruff linter, and you will have to add an entry in the changelog for the checks to pass :)

@jf---
Copy link
Contributor Author

jf--- commented Dec 4, 2025

thx for the review @tomvanmele really cool let me get back to you

@jf---
Copy link
Contributor Author

jf--- commented Dec 5, 2025

image

so let me keep numpy arrays 32bit to avoid this warning showing up

@jf---
Copy link
Contributor Author

jf--- commented Dec 5, 2025

@tomvanmele seems in pretty good shape now

@tomvanmele
Copy link
Member

tomvanmele commented Dec 5, 2025

yes indeed, but the workflows will keep failing because of src/compas_notebook/scene/__init__.py

Screenshot 2025-12-05 at 14 06 12

either put back the __all__ or add an ignore flag to the file...

@jf---
Copy link
Contributor Author

jf--- commented Dec 5, 2025 •
edited
Loading

__all__ is a particular bad idea:

  • __all__ forces explicit public API like public Foo vs private Foo
  • __all__ is declarative and static in nature
  • __all__ restricts from module import * (similar to Java package exports)
  • __all__ is structured, controlled, API-boundary-oriented

it really encourages bugs, since its so misaligned with python dynamism

did I mention I dislike it 😈🧐😜

can I suggest in pyproject.toml:

[tool.ruff]
ignore = ["F401", "F403"]  # F403 = wildcard import, F401 = unused import

@tomvanmele
Copy link
Member

tomvanmele commented Dec 5, 2025 •
edited
Loading

the __all__s can definitely go away, and we can indeed add the F401 ignore flag.
i would prefer not to add F403, because there should not be any wildcard imports in library code in my opinion.

as for the PR.
not sure adding the flag to pyproject.toml will make a difference.
i think the workflow will not take it into account.
however, at least for now, you can add # ruff: noqa: F401 at the top of the __init__ file to make the problem go away.

how about that?

@tomvanmele
Clean up unused scene object imports
2ff3694 
Removed unused imports for various scene object types.
@jf---
Copy link
Contributor Author

jf--- commented Dec 5, 2025

green amazing thx @tomvanmele

@tomvanmele tomvanmele merged commit 34cfe79 into compas-dev:main Dec 5, 2025
11 checks passed
@tomvanmele
Copy link
Member

tomvanmele commented Dec 5, 2025

@jf--- should i cut a new release?

@jf---
Copy link
Contributor Author

jf--- commented Dec 7, 2025

yeah why not that's a nice idea...
should we advance with compas_occ / brep support?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomvanmele tomvanmele tomvanmele left review comments

Assignees

@tomvanmele tomvanmele

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jf--- @tomvanmele