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

Add RenderInstanceHelper for doing visual-only (non-physics) scenes #2539

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eundersander
Copy link
Contributor

Motivation and Context

DO NOT MERGE: I will make sure the v0.3.3 release is out the door before merging.

RenderInstanceHelper lets user python code add render asset instances to the Habitat visual scene and pose them with an efficient numpy batch interface (RenderInstanceHelper.set_world_poses).

The overall interface is intentionally minimal, so as to simplify porting the backend to other renderers like the experimental batch renderer.

In terms of specifying the render asset instance, we currently only offer 1. asset filepath (e.g. a GLB file) and 2. a semantic ID. We can expand this in the future, e.g. scale and material overrides. I'm avoiding using AssetInfo and RenderAssetInstanceCreationInfo at the python level because I don't think all the options should be exposed to python users.

How Has This Been Tested

Added a test to SimTest.cpp. Also tested locally in an example HITL app that lives outside Habitat-sim.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@eundersander eundersander added the do not merge Not ready to merge. This label should block merging. label Jan 29, 2025
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 29, 2025
Copy link
Contributor Author

@eundersander eundersander left a comment

Choose a reason for hiding this comment

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

Some comments for reviewers

return Mn::Image2D{Mn::PixelFormat::RGBA8Unorm, size, std::move(outputData)};
}

void SimTest::checkPinholeCameraSemanticObservation(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should find some code re-use with checkPinholeCameraRGBAObservation.

py::class_<RenderInstanceHelper, RenderInstanceHelper::ptr>(
m, "RenderInstanceHelper")
.def(py::init<sim::Simulator&, bool>(), py::arg("sim"),
py::arg("use_xyzw_orientations"), "todo")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll replace the todos with documentation before merging.

#include "esp/sim/Simulator.h"
#include "esp/sim/SimulatorConfiguration.h"

namespace py = pybind11;
using py::literals::operator""_a;

namespace {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should probably move this to a pybind_utils.cpp inside our bindings library.

.def("get_num_instances", &RenderInstanceHelper::GetNumInstances, "todo")
.def(
"set_world_poses",
[](RenderInstanceHelper& self, py::array_t<float> positions,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why all this complexity in the binding code?

  1. I want to allow users to directly pass their numpy arrays into C++, with no memory copy.
  2. Pybind/numpy C++ code pretty much must be directly in our bindings library (which uses pybind11_add_module), not other parts of Habitat-sim.

namespace esp {
namespace sim {

RenderInstanceHelper::RenderInstanceHelper(Simulator& sim,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add standard method documentation before merging.

namespace sim {

RenderInstanceHelper::RenderInstanceHelper(Simulator& sim,
bool useXYZWOrientations) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Give users the choice of quat convention and force them to declare it.

namespace esp {
namespace sim {

class RenderInstanceHelper {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add standard class documentation before merging.

@@ -245,11 +266,89 @@ void SimTest::reset() {
CORRADE_COMPARE(pathfinder, simulator->getPathFinder());
}

Mn::Image2D convertR32uiToRgba8Unorm(const Mn::ImageView2D& inputImage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I can find a better home for this helper function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. do not merge Not ready to merge. This label should block merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants