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

Complete geometry.py module: handling sphere and cylinder #7

Merged
merged 51 commits into from
Jun 18, 2024

Conversation

skim0119
Copy link
Collaborator

@skim0119 skim0119 commented May 31, 2024

Changes

Here, the goal is to refactor geometry modules and complete them.

TODO

  • checkout the branch wip/geometry
  • First take a look at BlenderMeshInterfaceProtocol. Make a unit-test for bsr.geometry.Sphere and bsr.geometry.Cylinder based on the fact that objects of these two classes must conform the BlenderMeshInterfaceProtocol protocol.
  • Make a modification to cylinder module. I already made Sphere module, so take a look at the structure.
  • Fill in documentation, and check the build.
  • Commit and push the change, and ask for review

Note

@Rohar10 I finished Sphere module as an example. You can take a look and finish type-hinting Cylinder module.

Protocol: https://dev.to/shameerchagani/what-is-a-protocol-in-python-3fl1
The idea behind the protocol is to specify the behavior of a python object. Take a look at BlenderMeshInterfaceProtocol: any bsr object that is supposed to interface a primitive blender object (i.e. sphere, cylinder, etc) must conform a protocol specified by BlenderMeshInterfaceProtocol. In this case, it is important to have a constructor, getter for object, getter for states, and method to update states.

How to check

  • pass make test
  • pass make mypy
  • check make docs

@skim0119 skim0119 added documentation Improvements or additions to documentation enhancement New feature or request labels May 31, 2024
@skim0119 skim0119 self-assigned this May 31, 2024
@hanson-hschang
Copy link
Collaborator

hanson-hschang commented Jun 4, 2024

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L25

This definition of P is not used later on in the code.

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L6-L15

Two imports are not used: TypedDict and Self. In addition, ParamSpec is not used if P is not used.

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L24-L28

The nameing: I think it is better to short the class name to: BlenderMeshProtocol or BlenderMeshInterface, since setting up protocols means creating interfaces. Personally, I prefer BlenderMeshProtocol. In addition, is S a good naming? I am not sure what is the convention here... (capitalized single alphabet? Maybe O stands for Object in Blender? or use Obj? M for mesh? or Mesh? or could you explain more on why choose this naming S?)

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L71-L73

The return type of this class method. Why is it a string "Sphere" not the class Sphere? In addition, why define this class method create, rather than directly use __init__? I am asking this because what happen if the states does not have the keys: position or radius? I thought the use of __init__ forces the user to give these two arguments.

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L176-L185

I don't understand what TYPE_CHECKING is doing ... Do you have reference to this? Is this a python thing or a mypy thing? I know that the TYPE_CHECKING constant defined by the typing module is False at runtime but True while type checking. But what's the purpose of this if block?

@skim0119
Copy link
Collaborator Author

skim0119 commented Jun 4, 2024

@hanson-hschang

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L25

This definition of P is not used later on in the code.

Okay, but we need it later.

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L6-L15

Two imports are not used: TypedDict and Self. In addition, ParamSpec is not used if P is not used.

Again, need them later.

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L24-L28

The nameing: I think it is better to short the class name to: BlenderMeshProtocol or BlenderMeshInterface, since setting up protocols means creating interfaces. Personally, I prefer BlenderMeshProtocol. In addition, is S a good naming? I am not sure what is the convention here... (capitalized single alphabet? Maybe O stands for Object in Blender? or use Obj? M for mesh? or Mesh? or could you explain more on why choose this naming S?)

We'll add ...Protocol for any protocols to avoid confusion between other namings. Same for ...Type. I think BlenderMesh is misleading because it is not a mesh. BlenderMeshInterfaceProtocol is correct term here. I'm open for discussing if we want to append ...P, such that it is BlenderMeshInterfaceP instead. I've seen some code using this rule, but they doesn't look so clean. Up for discussion.

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L71-L73

The return type of this class method. Why is it a string "Sphere" not the class Sphere?

Because Sphere is not defined at this line, and python cause error. I think it is resolved in later version of python, but at this moment we just need to use string format.

In addition, why define this class method create, rather than directly use __init__? I am asking this because what happen if the states does not have the keys: position or radius? I thought the use of __init__ forces the user to give these two arguments.

I think these interfaces will have multiple ways of constructing in the future, and I am using factory pattern. For example, we can have multiple way of making spheres in blender, and user might need to choose one or other. In case of rod, I already see that we need cylinder+sphere method for debugging/diagnostics, and bezier/B-spline for visual. We can change it later, but using @classmethod is easier to extend in the future.

https://github.com/GazzolaLab/Blender_Soft_Rod_Visualizer/blob/10fc24205ba28557a513769f2fd726583af0b0b3/src/bsr/geometry.py#L176-L185

I don't understand what TYPE_CHECKING is doing ... Do you have a reference to this? Is this a python thing or a mypy thing? I know that the TYPE_CHECKING constant defined by the typing module is False at runtime but True while type checking. But what's the purpose of this if block?

Idea is that this block is only visible by mypy. I want Sphere and Cylinder object to conform BlenderMeshInterfaceProtocol, but there is no explicit way of testing it. It is a trick introduced here.

@hanson-hschang
Copy link
Collaborator

@skim0119

I'm open for discussing if we want to append ...P, such that it is BlenderMeshInterfaceP instead. I've seen some code using this rule, but they don't look so clean. Up for discussion.

...P doesn't looks good to me. That's stick to ...Protocol.

In addition, is S a good naming? I am not sure what is the convention here... (capitalized single alphabet? Maybe O stands for Object in Blender? or use Obj? M for mesh? or Mesh? or could you explain more on why choose this naming S?)

How about this part? What is S and P used for?

@skim0119
Copy link
Collaborator Author

skim0119 commented Jun 4, 2024

The letter doesn't have much meaning. Capital single letter is typically used for generic typing in c++ image, and that is why people use T or U or some capital letter

Copy link
Collaborator

@hanson-hschang hanson-hschang left a comment

Choose a reason for hiding this comment

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

LGTM

@skim0119 skim0119 removed the request for review from Rohar10 June 7, 2024 22:13
src/bsr/geometry.py Outdated Show resolved Hide resolved
@Rohar10
Copy link
Collaborator

Rohar10 commented Jun 18, 2024

Cylinder and Sphere modules complete, passing all test cases. Documentation attached here.
Screenshot 2024-06-18 at 12 14 25 AM
Screenshot 2024-06-18 at 12 14 37 AM
Screenshot 2024-06-18 at 12 14 48 AM
Screenshot 2024-06-18 at 12 14 56 AM
Screenshot 2024-06-18 at 12 15 08 AM
file:///Users/rohitharish/github/Blender-Soft-Arm-Simulation/docs/build/html/api/geometry.html#bsr.geometry.Cylinder

Copy link
Collaborator Author

@skim0119 skim0119 left a comment

Choose a reason for hiding this comment

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

LGTM

@skim0119 skim0119 merged commit 75d7e3f into main Jun 18, 2024
1 check passed
@skim0119 skim0119 deleted the wip/geometry branch June 18, 2024 14:50
skim0119 added a commit that referenced this pull request Jun 18, 2024
Complete `geometry.py` module: handling sphere and cylinder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants