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

Surface API alternative PR #1375

Merged
merged 11 commits into from
Jul 4, 2024
Merged

Surface API alternative PR #1375

merged 11 commits into from
Jul 4, 2024

Conversation

tomvanmele
Copy link
Member

This PR is a refactored version of #1350 ...

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 50.54945% with 45 lines in your changes missing coverage. Please review.

Project coverage is 59.92%. Comparing base (47eb7ee) to head (6df427b).

Files Patch % Lines
src/compas/geometry/surfaces/nurbs.py 48.78% 42 Missing ⚠️
src/compas/geometry/surfaces/surface.py 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1375      +/-   ##
==========================================
- Coverage   59.99%   59.92%   -0.07%     
==========================================
  Files         206      206              
  Lines       22115    22151      +36     
==========================================
+ Hits        13267    13275       +8     
- Misses       8848     8876      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomvanmele
Copy link
Member Author

Some explanation...

  • compas.geometry.surfaces.surface.Surface is like an ABC for parametric surfaces.
  • compas.geometry.surfaces.surface.Surface.__new__ has been replaced such that only subclasses can be instantiated.
  • __new__ re-overwrites of subclasses are therefore no longer necessary.
  • Surface has many implementations (Conical, Cylindrical, Planar, Spherical, Toroidal, Nurbs). only NurbsSurface requires a plugin implementation.
  • from_native has been added to Surface since CAD engines can provide a "native" surface from which all parametric functionality can be derived, even without knowing what kind of surface it is.
  • all specific factory methods have been moved to NurbsSurface since they indeed produce a Nurbs surface.
  • elementary surfaces can be instantiated directly from the specific pure Python classes (ConicalSurface, CylindricalSurface, ...)
  • a few small bugs have been fixed along the way.
  • similar changes should be made to the Curve API...

all of this has been tested with OCC as plugin and using the viewer for visualisation. Rhino should still be verified.

@@ -109,25 +144,46 @@ def __data__(self):

@classmethod
def __from_data__(cls, data):
"""Construct a Nurbs surface from its data representation.
Copy link
Member

Choose a reason for hiding this comment

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

is the domain not part of the representation? can it somehow be inferred from another piece of data?

Co-authored-by: Chen Kasirer <ckasirer@ethz.ch>

@classmethod
def from_interpolation(cls, points, *args, **kwargs):
"""Construct a surface from a frame.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Construct a surface from a frame.
"""Construct a surface from an interpolation.


"""
return new_nurbssurface_from_native(cls, surface)
raise NotImplementedError
Copy link
Member

Choose a reason for hiding this comment

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

call plugin here?

@@ -51,7 +45,9 @@ class Surface(Geometry):
"""

def __new__(cls, *args, **kwargs):
return new_surface(cls, *args, **kwargs)
if cls is Surface:
raise TypeError("Instantiating the base Surface class directly is not allowed.")
Copy link
Member

Choose a reason for hiding this comment

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

I would maybe change the wording a bit. the caller doesn't necessarily know an entirely different class is gonna be created, that's abstracted by the plugin system. if I saw something like this I'd go looking for the right child class to instantiate myself. Instead, they should know the default constructor is not allowed but the other ones yes.

Copy link
Member

@chenkasirer chenkasirer left a comment

Choose a reason for hiding this comment

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

LGTM!

@chenkasirer chenkasirer mentioned this pull request Jul 4, 2024
10 tasks
@tomvanmele tomvanmele merged commit 8007489 into main Jul 4, 2024
15 of 17 checks passed
@tomvanmele tomvanmele deleted the surface-api-alternative-pr branch July 4, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants