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

Replay Batch Renderer #1904

Merged
merged 10 commits into from
Oct 26, 2022
Merged

Replay Batch Renderer #1904

merged 10 commits into from
Oct 26, 2022

Conversation

0mdc
Copy link
Contributor

@0mdc 0mdc commented Oct 19, 2022

Motivation and Context

This changeset is a continuation of the work started in #1745 by @eundersander (see facebookresearch/habitat-lab#863 for full context).

How Has This Been Tested

ReplayBatchRendererTest is successful.

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.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 19, 2022
Copy link
Contributor

@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.

LGTM!

@mosra
Copy link
Collaborator

mosra commented Oct 24, 2022

Just making sure -- is this meant to eventually also abstract away from using a SceneNode or that's to be done in a later PR?

On my side I have some really dirty code that attempts to make that abstraction, by storing an ID inside a SceneNode*. If you would find it useful, I can share the patch.

@0mdc
Copy link
Contributor Author

0mdc commented Oct 24, 2022

@mosra

Just making sure -- is this meant to eventually also abstract away from using a SceneNode or that's to be done in a later PR?

This is intended to help abstracting away the scene graph, however this will be done in a later PR.

On my side I have some really dirty code that attempts to make that abstraction, by storing an ID inside a SceneNode*. If you would find it useful, I can share the patch.

I'd love to see your vision on this.

@0mdc 0mdc mentioned this pull request Oct 24, 2022
11 tasks
@0mdc 0mdc changed the title [WIP] Replay Batch Renderer Replay Batch Renderer Oct 25, 2022
Comment on lines +142 to +143
std::vector<std::vector<char>> buffers(numEnvs);
std::vector<Mn::MutableImageView2D> imageViews;
Copy link
Collaborator

@mosra mosra Oct 25, 2022

Choose a reason for hiding this comment

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

FYI you could use Magnum's Image2D instead of a buffer + an image view. It's an owning container so there's no need to maintain a separate buffer for it, and it's implicitly convertible to a [mutable]ImageView2D as well.

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 move on with this changeset and improve the test case when the real batch renderer will be integrated here.

@0mdc 0mdc merged commit c04ec0d into facebookresearch:main Oct 26, 2022
@0mdc 0mdc deleted the batch_renderer_wip branch June 14, 2023 01:57
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants