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

RenderingEndPoint #1147

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from
Draft

Conversation

gralkapk
Copy link
Member

Replace SplitView with RenderingEndPoints in ImGui (requires docking feature).

Summary of Changes

  • Infrastructure for passing resources to windows

Infrastructure for passing resources to windows
@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

Comment on lines 45 to 47
ep_v.entry_point_data->update();

ep_v.execute(ep_v.modulePtr, ep_v.entry_point_resources, ep_v.execution_result_image);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already done by the image presentation service once a framce for each registered entry point. Triggering re-rendering of a frame from inside the GUI seems a bad idea. The intended approach is to then use the ep_v.execution_result_image texture handle (either GL or CPU texture) to present the rendered texture in an ImGui window.

Copy link
Contributor

Choose a reason for hiding this comment

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

The rendering execution result image is only available after entry point/graph execution, which introduces a recursive dependency on showing the entry point results in the GUI, which itself is rendered as an entry point.

Comment on lines 45 to 47
ep_v.entry_point_data->update();

ep_v.execute(ep_v.modulePtr, ep_v.entry_point_resources, ep_v.execution_result_image);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose dragging the frontend resources up until here would also be not necessary without executing the entry points?

@gralkapk gralkapk marked this pull request as draft November 30, 2022 16:57
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

github-actions bot commented Dec 1, 2022

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@gralkapk
Copy link
Member Author

/format

#include "AbstractWindow.h"

#include "ImagePresentationEntryPoints.h"
#include "ImagePresentation_Service.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

This dependency between GUI (GUI Service) and Image Presentation Service may lead to problems. For cleaner separation of code it will be cleaner to pull the dependencies from ImagePresentation_Service.hpp into a separate header or otherwise restructure the affected code.

wrapped_images.push_back(entry.execution_result_image);
auto const& sink_list = ep_sink_map[entry.moduleName];
for (auto const& sink : sink_list) {
sink_img_map[sink].push_back(entry.execution_result_image);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about the details but it might lead to many GL texture / Byte texture copy operations when putting ImageWrapper execution_result_image in different places. I believe the initial goal for the ImageWrapper was to have a lightweight abstraction over GL/Byte textures with the option to transform one into the other (texture down/upload). It may be that the ImageWrapper copy constructor also copies textures/vectors internally, depending on whether the held texture is only referenced or actually owned.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@gralkapk
Copy link
Member Author

/format

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@gralkapk
Copy link
Member Author

/format

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants