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

py geometry: Expose RenderEngine API and trampoline for inheritance #13835

Merged

EricCousineau-TRI merged 1 commit into RobotLocomotion:master from EricCousineau-TRI:issue-13834-py-render-engine

Oct 21, 2020

Contributor

EricCousineau-TRI commented Aug 6, 2020 •

edited by sherm1

Resolves #13834

FYI @thduynguyen and @SeanCurtis-TRI Review will be coming y'all's way once this is ready ;)

This change is

Contributor Author

EricCousineau-TRI commented Aug 6, 2020

@thduynguyen Handing off to you for completion. Feel free to push here and/or start a separate PR.

EricCousineau-TRI assigned thduynguyen

EricCousineau-TRI commented

View reviewed changes

Contributor Author

EricCousineau-TRI left a comment

+@thduynguyen for finishing up

Reviewable status: LGTM missing from assignee thduynguyen, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @thduynguyen)

Contributor Author

EricCousineau-TRI commented Aug 26, 2020

See PR: EricCousineau-TRI#13

Contributor Author

EricCousineau-TRI commented Oct 19, 2020

Tagging @takuya-ikeda-tri

Contributor Author

EricCousineau-TRI commented Oct 20, 2020

@thduynguyen Does this happen to have your latest changes? (I just merged your PR in)

Contributor Author

EricCousineau-TRI commented Oct 20, 2020

From Duy:
thduynguyen@fbe17ba

EricCousineau-TRI force-pushed the issue-13834-py-render-engine branch 2 times, most recently from fe08ebf to d05a866 Compare

October 20, 2020 22:04

EricCousineau-TRI marked this pull request as ready for review

October 20, 2020 22:04

EricCousineau-TRI assigned SeanCurtis-TRI

EricCousineau-TRI commented

View reviewed changes

Contributor Author

EricCousineau-TRI left a comment

+@thduynguyen for feature review, please
+@SeanCurtis-TRI for platform review, please

Reviewable status: LGTM missing from assignees SeanCurtis-TRI(platform),thduynguyen (waiting on @SeanCurtis-TRI and @thduynguyen)

thduynguyen reviewed

View reviewed changes

Contributor

thduynguyen left a comment

with some nits. Thanks!

Reviewed 1 of 2 files at r1.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @EricCousineau-TRI, @SeanCurtis-TRI, and @thduynguyen)

bindings/pydrake/test/geometry_test.py, line 18 at r1 (raw file):

from pydrake.common.value import AbstractValue, Value
from pydrake.lcm import DrakeLcm, Subscriber
from pydrake.math import RigidTransform, RigidTransform_

BTW, what's the difference between RigidTransform and RigidTransform_? There are many re-definitions of RigidTransform = RigidTransform_[float] below in this file and that causes confusion. Can you either remove this or change the aliases to RigidTransformf?

bindings/pydrake/test/geometry_test.py, line 636 at r1 (raw file):

        Value[mut.render.RenderLabel]

    def test_render_camera_api(self):

BTW, are these 2 lines needed?

bindings/pydrake/test/geometry_test.py, line 643 at r1 (raw file):

            """Mirror of C++ DummyRenderEngine."""

            latest_intance = None

nit, variable name typo: latest_instance

bindings/pydrake/test/geometry_test.py, line 643 at r1 (raw file):

            """Mirror of C++ DummyRenderEngine."""

            latest_intance = None

nit, can you add a comment here stating this's only needed for testing (and why)?

bindings/pydrake/test/geometry_test.py, line 682 at r1 (raw file):

            def ImplementGeometry(self, shape, user_data):
                DummyRenderEngine.latest_instance = self
                pass

nit, redundant pass.

bindings/pydrake/test/geometry_test.py, line 703 at r1 (raw file):

            def DoClone(self):
                DummyRenderEngine.latest_instance = self

nit, shouldn't latest_instance = new in this case?

EricCousineau-TRI force-pushed the issue-13834-py-render-engine branch from d05a866 to 0496e02 Compare

October 21, 2020 19:46

EricCousineau-TRI commented

View reviewed changes

Contributor Author

EricCousineau-TRI left a comment

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @SeanCurtis-TRI and @thduynguyen)

bindings/pydrake/test/geometry_test.py, line 18 at r1 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

BTW, what's the difference between RigidTransform and RigidTransform_? There are many re-definitions of RigidTransform = RigidTransform_[float] below in this file and that causes confusion. Can you either remove this or change the aliases to RigidTransformf?

OK I'd like to keep it as-is for now. See here:
https://drake.mit.edu/python_bindings.html#differences-with-c-api

(Could consider as a separate PR focused on that, but I wanna punt for now ;)

bindings/pydrake/test/geometry_test.py, line 636 at r1 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

BTW, are these 2 lines needed?

Done. Oops!

bindings/pydrake/test/geometry_test.py, line 643 at r1 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

nit, variable name typo: latest_instance

Done. Oops as well!

bindings/pydrake/test/geometry_test.py, line 643 at r1 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

nit, can you add a comment here stating this's only needed for testing (and why)?

Done.

bindings/pydrake/test/geometry_test.py, line 682 at r1 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

nit, redundant pass.

Done.

bindings/pydrake/test/geometry_test.py, line 703 at r1 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

nit, shouldn't latest_instance = new in this case?

OK Nah, that smells a tad odd. I can explain more if you like.

EricCousineau-TRI force-pushed the issue-13834-py-render-engine branch from 0496e02 to c2eea45 Compare

October 21, 2020 19:48

EricCousineau-TRI mentioned this pull request

py geometry_test: Fix existing flake8 defects #14233

Merged

EricCousineau-TRI force-pushed the issue-13834-py-render-engine branch from c2eea45 to 030fcd0 Compare

October 21, 2020 19:53

EricCousineau-TRI assigned sammy-tri and unassigned SeanCurtis-TRI

EricCousineau-TRI commented

View reviewed changes

Contributor Author

EricCousineau-TRI left a comment

-@SeanCurtis-TRI +@sammy-tri for platform, please!

Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri and @thduynguyen)

EricCousineau-TRI mentioned this pull request

py geometry: Should permit RenderEngine to be extended in Python #13834

Closed

EricCousineau-TRI force-pushed the issue-13834-py-render-engine branch from 030fcd0 to 88c334b Compare

October 21, 2020 21:48

Contributor Author

EricCousineau-TRI commented Oct 21, 2020

Removed excess bindings / using statements, and made a comment here: #13834 (comment)

PTAL!

Contributor Author

EricCousineau-TRI commented Oct 21, 2020

Er... On second though, mayhaps I should keep the pure virtual NVI methods?
Otherwise, they won't appear in Sphinx docs....

sammy-tri reviewed

View reviewed changes

Contributor

sammy-tri left a comment

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @EricCousineau-TRI)

bindings/pydrake/geometry_py.cc, line 316 at r3 (raw file):

                &Class::default_render_label),
            cls_doc.default_render_label.doc)
        .def("GetRenderLabelOrThrow",

is it worth adding a comment explaining that we're deliberately binding to the trampoline even though https://pybind11.readthedocs.io/en/stable/advanced/classes.html#overriding-virtual-functions-in-python says not to? (I assume it's something to do with moving these functions into the public section).


          py geometry: Expose RenderEngine API and trampoline for inheritance

078ad14

Co-Authored-By: Duy-Nguyen Ta <duynguyen.ta@tri.global>

EricCousineau-TRI force-pushed the issue-13834-py-render-engine branch from 88c334b to 078ad14 Compare

October 21, 2020 21:58

EricCousineau-TRI commented

View reviewed changes

Contributor Author

EricCousineau-TRI left a comment

Per Slack, will keep the pure virtual NVI methods out for now. Yes, there may be a lack of docs; we can always come back and cherry-pick from r2 if need be.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)

bindings/pydrake/geometry_py.cc, line 316 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

is it worth adding a comment explaining that we're deliberately binding to the trampoline even though https://pybind11.readthedocs.io/en/stable/advanced/classes.html#overriding-virtual-functions-in-python says not to? (I assume it's something to do with moving these functions into the public section).

Done. Thanks!

sammy-tri reviewed

View reviewed changes

Contributor

sammy-tri left a comment

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),thduynguyen

Contributor Author

EricCousineau-TRI commented Oct 21, 2020

Skipping slow build

EricCousineau-TRI merged commit 9b7a97b into RobotLocomotion:master

SeanCurtis-TRI mentioned this pull request

camera_properties: Support setting with intrinsic matrix, possibly using CameraInfo #11880

Closed

Contributor

SeanCurtis-TRI commented Nov 6, 2020

@thduynguyen @EricCousineau-TRI

In looking at the bindings in PyRenderEngine, we've bound the API that should be deprecated (the ones that take CameraProperties and DepthCameraProperties). Life would be easiest if we can simply kill those APIs from the bindings. All rendering logic should be in the DoRender*Image methods and not in the Render*Image methods.

Given it's only been 16 days since the merge, do you think we can get away with this (without deprecation, that is)?

Contributor Author

EricCousineau-TRI commented Nov 6, 2020

Yeah, I'd be fine with that!

SeanCurtis-TRI mentioned this pull request

[pydrake] Update RenderEngine python trampoline - omit soon-to-be-removed API #14300

Merged

EricCousineau-TRI mentioned this pull request

py geometry: Fix bindings for RenderCamera classes #14394

Merged

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