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.

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

Add ViewportStreamer Gem #96

Merged
merged 2 commits into from
Jan 20, 2025
Merged

Add ViewportStreamer Gem #96

merged 2 commits into from
Jan 20, 2025

Conversation

lawendah
Copy link
Contributor

This PR adds a ViewportStreamer Gem providing a sensor, which publishes current viewport image. Viewport image is equivalent to the last frame rendered by current pipeline in default viewport context. Image data is published on sensor_msgs::msg::Image ROS2 topic.

MateuszWasilewski
MateuszWasilewski reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@MateuszWasilewski MateuszWasilewski left a comment

Choose a reason for hiding this comment

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

I think ViewportStreamerComponent should be a system component that is active in Game Mode only.
Other comments regard simplifying the code and handling potential nullptr.

Gems/ViewportStreamer/Code/Source/Clients/ViewportStreamerComponent.h Outdated

namespace ViewportStreamer
{
class ViewportStreamerComponent : public ROS2::ROS2SensorComponentBase<ROS2::TickBasedSource>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see benefit to using ROS2SensorComponentBase here. New Viewport images are generated with every OnTick event. We can simply connect to TickBus.

Copy link

Choose a reason for hiding this comment

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

As we discussed offline, you can also use TickBasedSource or even EventSourceAdapter, if you would like adapted frequency != tick bus frequency. However, in this case the easiest way would be to use TickBus directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After all I implemented it as EventSourceAdapter due to custom frequency configurable from setreg.

Gems/ViewportStreamer/Code/Source/Clients/ViewportStreamerComponent.cpp Outdated
Comment on lines 63 to 72
AZStd::vector<AZStd::string> passHierarchy;
auto pipelineHierarchy = pipeline->GetRootPass()->GetPathName().GetCStr();
std::string substr;
std::istringstream pipelineStr(pipelineHierarchy);

while (std::getline(pipelineStr, substr, '.'))
{
passHierarchy.push_back(AZStd::string(substr.c_str()));
}
passHierarchy.push_back("CopyToSwapChain");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why is this logic needed?
In https://github.com/o3de/o3de/blob/40cdf65119022498d9e61b9b021d461cf05bdc32/Gems/Atom/Tools/AtomToolsFramework/Code/Source/PreviewRenderer/PreviewRenderer.cpp#L84 pass hierarchy always stays the same for the same pipeline without any splitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic arose from the output of GetPathName() where passes are separated by the dot, e.g. Root.MainPipeline_-10. Putting this raw in pass hierarchy always yields an error Failed to find pass from Root.MainPipeline_-10. I am not able compare this with different usage of pass hierarchy. I am open to any suggestions on how to overcome this case in a different way, if you have one.

Gems/ViewportStreamer/Code/Source/Clients/ViewportStreamerComponent.cpp Outdated
Comment on lines 31 to 41
const AZStd::unordered_map<AZ::RHI::Format, const char*> FormatMappings{
{ AZ::RHI::Format::R8G8B8A8_UNORM, "rgba8" },
{ AZ::RHI::Format::B8G8R8A8_UNORM, "bgra8" },
{ AZ::RHI::Format::R32_FLOAT, "32FC1" },
};

const AZStd::unordered_map<AZ::RHI::Format, int> BitDepth{
{ AZ::RHI::Format::R8G8B8A8_UNORM, 4 * sizeof(uint8_t) },
{ AZ::RHI::Format::B8G8R8A8_UNORM, 4 * sizeof(uint8_t) },
{ AZ::RHI::Format::R32_FLOAT, sizeof(float) },
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't R8G8B8A8_UNORM the only one format used by CopyToSwapchain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my tests cases, the B8G8R8A8_UNORM format occured frequently. If you point the R8G8B8A8_UNORM I would retain these two and remove R32_FLOAT which is clearly redundant.

PawelLiberadzki
PawelLiberadzki reviewed Jan 14, 2025
View reviewed changes
Copy link

@PawelLiberadzki PawelLiberadzki left a comment

Choose a reason for hiding this comment

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

Just minor remarks from my side. However, I have run it and it works as expected.

Gems/ViewportStreamer/Code/Source/Clients/ViewportStreamerComponent.h Outdated

namespace ViewportStreamer
{
class ViewportStreamerComponent : public ROS2::ROS2SensorComponentBase<ROS2::TickBasedSource>
Copy link

Choose a reason for hiding this comment

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

As we discussed offline, you can also use TickBasedSource or even EventSourceAdapter, if you would like adapted frequency != tick bus frequency. However, in this case the easiest way would be to use TickBus directly.

@lawendah lawendah force-pushed the feature/viewport_streamer branch 2 times, most recently from 5878e4d to 21d5eef Compare January 16, 2025 15:05
PawelLiberadzki
PawelLiberadzki reviewed Jan 20, 2025
View reviewed changes
Copy link

@PawelLiberadzki PawelLiberadzki left a comment

Choose a reason for hiding this comment

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

All requested changes made. I have few additional comments to the updated part. Moreover, we discussing the runtime issue offline.

@lawendah lawendah force-pushed the feature/viewport_streamer branch from 21d5eef to 650f02c Compare January 20, 2025 12:47
@lawendah lawendah force-pushed the feature/viewport_streamer branch from 650f02c to 93ac2e3 Compare January 20, 2025 13:27
@lawendah lawendah force-pushed the feature/viewport_streamer branch from 93ac2e3 to f8ed5a9 Compare January 20, 2025 14:34
PawelLiberadzki
PawelLiberadzki approved these changes Jan 20, 2025
View reviewed changes
Copy link

@PawelLiberadzki PawelLiberadzki left a comment

Choose a reason for hiding this comment

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

Looks good! Check in runtime, both Editor and GameLauncher - works.

@lawendah lawendah merged commit a2a2c77 into main Jan 20, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PawelLiberadzki PawelLiberadzki PawelLiberadzki approved these changes

@MateuszWasilewski MateuszWasilewski Awaiting requested review from MateuszWasilewski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lawendah @PawelLiberadzki @MateuszWasilewski