search

compas-dev / compas_robots

Basic infrastructure for working with robots in COMPAS.
http://compas.dev/compas_robots/
MIT License
1 stars 4 forks source link

compas_viewer scene object #13

Closed ZacZhangzhuo closed 2 months ago

ZacZhangzhuo commented 4 months ago

see https://github.com/compas-dev/compas_viewer/pull/74

Major Changes

2024-02-02-29-15-58-43

Test

Example file can be found at : https://compas.dev/compas_viewer/latest/examples/object/robot.html

jf--- commented 3 months ago

@gonzalocasas or @tomvanmele anything keeping you back from merging? cool contribution @ZacZhangzhuo

gonzalocasas commented 3 months ago

@gonzalocasas or @tomvanmele anything keeping you back from merging? cool contribution @ZacZhangzhuo

thanks for the poke!

ZacZhangzhuo commented 3 months ago

@gonzalocasas or @tomvanmele anything keeping you back from merging? cool contribution @ZacZhangzhuo

thanks! :-)

I think we are good to go @gonzalocasas

ZacZhangzhuo commented 3 months ago

the build still failed even with:


@plugin(category="factories", requires=["compas_viewer"])
def register_scene_objects():
    from .robotmodelobject import RobotModelObject

    register(RobotModel, RobotModelObject, context="Viewer")

@gonzalocasas

gonzalocasas commented 3 months ago

Sorry, I just realized that something is off. And it's that we don't need to have the viewer plugin inside compas_robots, that's exactly why these things are plugins, so that this module can live inside the repo of compas_viewer and be auto-discovered

ZacZhangzhuo commented 3 months ago

Sorry, I just realized that something is off. And it's that we don't need to have the viewer plugin inside compas_robots, that's exactly why these things are plugins, so that this module can live inside the repo of compas_viewer and be auto-discovered

didn't really get it. How should we modify the code then? i created this init.py based on src\compas_robots\rhino\scene\__init__.py, as a template.

gonzalocasas commented 3 months ago

Sorry, I just realized that something is off. And it's that we don't need to have the viewer plugin inside compas_robots, that's exactly why these things are plugins, so that this module can live inside the repo of compas_viewer and be auto-discovered

didn't really get it. How should we modify the code then? i created this init.py based on src\compas_robots\rhino\scene\__init__.py, as a template.

The code is all correct, but I think it just needs to be moved to the compas_viewer project instead of here.

tomvanmele commented 3 months ago

@gonzalocasas is it not exactly the other way around?! since it is a plugin system, not everything needs to be implemented in the viewer (since the viewer can't know about all the packages that want to plug into it), but rather in the packages themselves? Rhino scene objects are also implemented per package and not all in compas_rhino, for the same reason. the viewer "auto discovers" all implementations of viewer scene object implementations across packages...

gonzalocasas commented 3 months ago

Alright, this one was really confusing to reason about. I thought about it a bunch, then discussed it also with @yck011522.

So, in terms of dependencies, this is correct. The plugin part takes care of decoupling the dependency from compas_robots to compas_viewer, and at the same time, if the interface on the viewer side is correct, viewer should not be dependent on compas_robots either. So that's all good.

In this diagram (directed dashed line represents a "A depends on B" relationship), it's highlighted how the sub-packages of this repository that deal with each CAD/viewer have dependencies on those things (Blender, Viewer and Rhino respectively), and also have a dependency on the RobotModel class of compas_robots: image

My confusion came from the fact of what does a developer of different parts of the system need to be aware of when developing:

We don't have this problem so clear in the case of Rhino/Blender, because those code bases are external to us, and none of us develops Rhino or Blender themselves, but viewer is a bit of a different case.

And here there's a bit of an assumption, but I expect the API of compas_robots and its RobotModel class to change more often than the base API that compas_viewer requires for it to be able to support a new object type, so, in that case, it makes sense that this stays in this repository.

Bottom line: we keep it here. The only thing remaining is to fix the build.

gonzalocasas commented 2 months ago

This one has been replaced by https://github.com/compas-dev/compas_robots/pull/14