add ImageComponent that creates and draws ImGui images

[timoore]

Add a compile() function to RenderImGui that compiles child resources like ImageComponent.

This is similar to the code I wrote for vsgCs. I'm not entirely happy that compilable resources in components need to be added directly to the RenderImGui object. It would be better if RenderImGui traversed its components during compile(), but at the moment the components are subclasses of a VSG class that has a compile() member function. Changing that would require more of an incompatible change than I'm willing to make without some discussion.

[timoore]

Crazy idea: RenderImGui becomes a subclass of vsg::Commands, the components are required to be subclasses of vsg::Command, all the ImGui stuff is done in record() member functions...

[robertosfield]

Crazy idea: RenderImGui becomes a subclass of vsg::Commands, the components are required to be subclasses of vsg::Command, all the ImGui stuff is done in record() member functions...

I've done a quick review of the PR so am still digesting it. Your crazy idea has me curious about the how well it could work + consequences. If you want to create a branch to experiment with this then I'd be up for considering it.

I do need to properly get my head around ImGui/vsgImGui, I keep do a bit of work on, leaving it for a month or two, then returning without ever using vsgImgGui directly in my own/client work. I feel it's with real-world usage you really get insight into how well functionality can work.

Another off the cuff suggestion. could we just have integrate the components more generally with the scene graph so one could use vsg::Switch etc. to toggle on/off subgraphs using switches/masks etc.

[robertosfield]

W.r.t crazy ideas, perhaps tt's better to review/merge this PR independent of any future refactoring of vsgImGui components to make more flexible. Thoughts @timoore?

[robertosfield]

If vsgImGui::RenderImGui was subclassed from vsg::Group and override the accept(RecordTraversal&) it could do the necessary ImGui API calls do in RenderImGui::recordComponents() and the fallow up calls in the RenderImGui::record() calls.

I don't know if there is a way of checking whether the ImDrawData is empty or not, but there is then we could possible get rid of the bool visibleComponents - we need this as we don't want an overhead when there is nothing to render.

Finally we'd need a way of conveniently providing the call to set up the calls to ImGui in place of the Component = std::function<bool()>. Having class like ImageComponent subclass from vsg::Command could be extended so that a Component subclasses from vsg::Command and "has a" std::function<bool()>.

Another element I have wondering about, more generally for the VSG, but usable here is how we put callbacks/event handlers into the scene graph. I haven't put general callbacks into the VSG so far as they add a memory and comparison during traversal on all nodes and most don't ever have callbacks so it;s mostly a performance drain. This is why we assign event handlers to the viewer to give us that event handling capability, but it's viewer level not scene graph level.

One approach I've considered is having callback nodes in the scene graph or extend the CompileTraversal so that it can not only do compilation of shaders, creation of vulkan objects and transfer data to GPU, but also be a way of class logging that it has a callback/event handler that needs to be called. The CompileTraversal could then be checked for event handlers etc. that should be called on each frame, There is a CompileResult container that is already used for dynamic data and other results so perhaps this could be the place to put these items.

This approach would allow us to more loosely couple the ImGui scene graph from the application setup, as the SendImGuiEvents could be automatically logged with the viewer..

[timoore]

If vsgImGui::RenderImGui was subclassed from vsg::Group and override the accept(RecordTraversal&) it could do the necessary ImGui API calls do in RenderImGui::recordComponents() and the fallow up calls in the RenderImGui::record() calls.

I forgot to mention another "bad smell" in this pull request: ImageComponent stores a ref to the Window object because it has no other way to get the device, and thence the descriptor set, when its draw code is called. I would very much like for RenderImGui to pass the context to its components, either in the current function call or by calling record().

I don't know if there is a way of checking whether the ImDrawData is empty or not, but there is then we could possible get rid of the bool visibleComponents - we need this as we don't want an overhead when there is nothing to render.

That's a good idea. It also might be worth checking if ImGui does anything at all if there's nothing in its draw list.

Finally we'd need a way of conveniently providing the call to set up the calls to ImGui in place of the Component = std::function<bool()>. Having class like ImageComponent subclass from vsg::Command could be extended so that a Component subclasses from vsg::Command and "has a" std::function<bool()>.

I'm not a huge fan of std::function and don't see the need here; is it offering anything that a required operator() function or record() wouldn't give us? Assuming we figure out a way to signal if there's been any ImGui recording.

Another element I have wondering about, more generally for the VSG, but usable here is how we put callbacks/event handlers into the scene graph. I haven't put general callbacks into the VSG so far as they add a memory and comparison during traversal on all nodes and most don't ever have callbacks so it;s mostly a performance drain. This is why we assign event handlers to the viewer to give us that event handling capability, but it's viewer level not scene graph level.

I've run into this issue in vsgCs. I use an update operation added to the viewer to call the tileset management that has to happen every frame, but this requires an additional step once the tileset is created; additionally, there's no good way to remove a task from the UpdateOperations queue, although I suppose I could dive in and make a subclass -- and pull request -- that supports this. One approach I've considered is having callback nodes in the scene graph or extend the CompileTraversal so that it can not only do compilation of shaders, creation of vulkan objects and transfer data to GPU, but also be a way of class logging that it has a callback/event handler that needs to be called. The CompileTraversal could then be checked for event handlers etc. that should be called on each frame, There is a CompileResult container that is already used for dynamic data and other results so perhaps this could be the place to put these items.

Yeah, vsg could have a protocol where a setup() method on a node is called the first time it is seen by a compile traversal. This could also be done by a designated object in the Auxiliary collection.

This approach would allow us to more loosely couple the ImGui scene graph from the application setup, as the SendImGuiEvents could be automatically logged with the viewer..

Tim

[timoore]

W.r.t crazy ideas, perhaps tt's better to review/merge this PR independent of any future refactoring of vsgImGui components to make more flexible. Thoughts @timoore?

If you think that the functionality is useful as at stands and don't mind that ImageComponent stores a ref to the Window, then I think it's fine to merge. I suppose that it's not to late to make breaking changes to RenderImGui in another pull request :metal:

[robertosfield]

I think the right thing to do is to merge this PR as a branch then cherry pick from it, or just use it as a reference.

Then create another branch of vsgImGui to trial refactoring it enable vsgImGui::Image/Component to subclassed from vsg::Command and have it handle it's own compile and have the record do the ImGui calls. I don't know whether to keep the Component as is for backwards compatibility or to have an adapter class this takes the std::function() as a member. Perhaps starting off with backwards compatibility is the thing to do.

Making RenderImGui subclass from vsg::Group opens the door to users subclassing from vsg::Command and doing their own ImGui class that way.

With the refactor I suspect the ImageComponent class could be simplified, so I think it makes sense to wait till a refactor before merging otherwise we'd just have a move the goal posts for users attempting to adopt it.

I'll merge as a branch now then experiment with a RenderImGui refactor this morning.

On the topic of UpdateOperators, a remove() method looks to be a a simple solution.

[robertosfield]

@timoore I've checked in a UpdateOperation::remove(ref_ptr opeator) method to VSG master.

https://github.com/vsg-dev/VulkanSceneGraph/commit/6b6790add08fb770dc6a2a91c6790d7549e54cca

[robertosfield]

I have merged as a branch:

https://github.com/vsg-dev/vsgImGui/tree/timoore-timoore/image-component

[robertosfield]

I've been investigating elements of a vsgImGui refactor and the draw_data->CmdListsCount value goes to 0 when nothing is going to be rendered so we can dispense with the bool returned from the RenderImGui::Component std::function and potentially just use standard vsg::Commands or other node subclasses.

We needn't subclass from vsg::Command in all cases as mostly there won't be any Vulkan objects to set up or dispatch directly, just special cases like setting up the DescriptorImage to get the TextureID.

I also want to remove the Window from ImageComponent, even right now it just need a Device so that's all you'd need to pass into the constructor, but preferable it wouldn't have this at all. All need do is record the deviceID that the texture was compiled for, so we can probably just cache that.

[robertosfield]

I think we can safely cache the deviceID in ImageComponent::compile(..) method as we can prevent it from being used with other devices with the following addition to RenderImGui:

void RenderImGui::record(vsg::CommandBuffer& commandBuffer) const { if (commandBuffer.deviceID != _device->deviceID) return;

This will also generalize to when RenderImGui is subclassed from vsg::Group.

The one area it would fail would be if the use added a ImageComponent to a different subgraph as well and this was compiled for another device. This is trying hard to find a way to break though...

[timoore]

In theory ImGui can be used in multiple windows / devices. Unless it's simple to pass the context into components, I'd do whatever to get it working.

On Wed, Feb 1, 2023 at 1:25 PM Robert Osfield @.***> wrote:

I think we can safely cache the deviceID in ImageComponent::compile(..) method as we can prevent it from being used with other devices with the following addition to RenderImGui:

void RenderImGui::record(vsg::CommandBuffer& commandBuffer) const { if (commandBuffer.deviceID != _device->deviceID) return;

This will also generalize to when RenderImGui is subclassed from vsg::Group.

The one area it would fail would be if the use added a ImageComponent to a different subgraph as well and this was compiled for another device. This is trying hard to find a way to break though...

— Reply to this email directly, view it on GitHub https://github.com/vsg-dev/vsgImGui/pull/45#issuecomment-1411977058, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABAQAJ4YQYYPT4S2YPPTFTWVJI5PANCNFSM6AAAAAAUMLSNQ4 . You are receiving this because you were mentioned.Message ID: @.***>

[robertosfield]

I have now checked in the first step at refactoring vsgImGui::RenderImGui, with a new RenderImGui_Refactor branch:

https://github.com/vsg-dev/vsgImGui/tree/RenderImGui_Refactor

The key change is RenderImGui now subclasses from vsg::Group rather than vsg::Command allowing users to do an addChild() to add their own nodes, so can subclass from vsg::Node for straight forward ImGui calls, or vsg::Command if they need to override the compile() method such as with ImageComponent.

I have left the old RenderImGui::Component API in place for backwards compatibility.

[robertosfield]

I don't know how easy it would be to get ImGui working with multiple Vulkan devices. For now using a separate RenderImGui for each device would be the way to tackle it.

Vulkan allows you to use multiple Windows with a single logical Device, you only need to create multiple Devices when the Windows on separate GPUs.

For now I'm happy to just focus on the single Window/Device case. Generalizing to multiple Window and one Device would be the next nature step. However, I have enough other work to get with chasing ImGui functionality is something I need to avoid getting to deeply drawn into.

[robertosfield]

@timoore could you let know what you want to do now w.r.t the ImageComponent branch. The RenderImGui_Refactor branch should now be ready to handle the addition of ImageComponent.

Perhaps a first step would simply be to merge the RenderImGui_Refactor branch with master and then merge this into the image component one, or simply create a new branch and copy in the ImageComponent class over to it.

Thoughts?

[timoore]

Go ahead and merge RenderImGui_Refactor. You can close this pull request and vsg-dev/vsgExamples#207; I'll resubmit them based on the refactored vsgImGui.

[robertosfield]

I'm merged with PR #46.

I'll leave this PR and the one on vsgExamples up, when you are ready and submit new ones I'll delete the older ones.

[robertosfield]

A have checked another experiment - using a ImGuiNode subclass from vsg::Node to adapt the Component function to a conventional node so can be added as child like any other node.

https://github.com/vsg-dev/vsgImGui/tree/ImGuiNode

This change should be orthogonal to your ImageComponent changes, but should put things on an even footing as we can then add the children in order underneath the RenderImGui.

We this change the naming of a std::function as a Component now feels even more odd so that is something we might want to change. All the changes I've made don't break backwards compatibility.

[robertosfield]

Some further evolution of the ImGuiNode with commit : 0236047d01d54908590fc1050704a6da55020f08

The key change is that the ImGuiNode has two lists of callback functions that can be either the legacy signature for backwads compatibility or a new RecordFunction one that the RecordTraversal is passed in:

/// std::function signature with RecordTraverasl used vsgImGui
using RecordFunction = std::function<void(vsg::RecordTraversal& rt)>;

/// std::function signature used for older vsgImGui versions
using LegacyFunction = std::function<bool()>;

I have changed the vsgimgui example to use the new std::function<void(vsg::RecordTraversal& rt) with a new vsgExamples ImGuiNode branch:

You can get the DeviceID from the RecordTraversal, though it's a bit long winded right now:

auto deviceID = rt.getState()._commandBuffer->deviceID;

I think it might be useful to cache the deviceID on RecordTraversal class to make it easier to access.

Another alternative it pass the RenderImGui node as a parameter to the RecordFunction.

https://github.com/vsg-dev/vsgExamples/tree/ImGuiNode

[robertosfield]

To make it easier to get the deviceID I've added:

    /// get the current CommandBuffer for current subgraph being traversed
    CommandBuffer* getCommandBuffer();

    /// get the current DeviceID for current subgraph being traversed
    uint32_t deviceID() const;

to vsg::RecordTraversal with commit : https://github.com/vsg-dev/VulkanSceneGraph/commit/a9410151c054ac28f0b7184436750a8e473b1e74

These changes should pave the way for get the TextureID a bit more conveniently.

With this I think I'm done for now refactoring for now so will get on with other work. Items I'm not happy about yet is the naming of RenderImGui and ImGuiNode, they work fine, but I'm wondering if there might be better naming lurking out there. Perhaps once we get the texture functionality integrated and used it'll all become clearer.

[robertosfield]

@timoore I have picked up the my work refactoring vsgImGui and integrating ImageComponent into the ImGuiNode branch, this is now far enough along I think it should be good enough to merge vsgImGui and vsgExamples master soon.

First integrated ImageComponent pretty cose to verbatim but have now started refactoring it a bit to make it more general purpose - such as removing the Window dependency. This changes usage a bit so I've updated the changes to vsgimgui_example:

https://github.com/vsg-dev/vsgImGui/commit/ed0e2f4ae859b20b052dc58297f3c348d5cd8af2 https://github.com/vsg-dev/vsgExamples/commit/319891698d41f3c5ca0a10438d6cf5aa7d0f44b6

I want to spend a bit more time with the code to see if there are further opportunities to make them more general purpose.

[robertosfield]

@timoore I have been thinking about the coupling between ImageComponent and an implementation of the ImGui function in the vsgimgui_example and it feels pretty clunky. Ideally we'd have things self contained so a class/function that handles the GUI also has responsibility for any textures than it wants to use.

I may mean that whole std::function approach won't cope this as we'll need to entry points - the RecordTraversal and the CompileTraversal. Unless we add support for doing compile in the RecordTraversal as well, which is a possibility.

I don't know where we'll end with this. What we have now is cludgy but workable, it might be that's an OK interim solution.

[robertosfield]

@timoore I have decided to simplify the public interface of RenderImGui and have simplified and moved the ImGuiNode class from the header into the RenderImGui.cpp source so now it's purely an implementation detailed used to make it possible to use RenderImGui as a group node. The commit is:

40f5837c5817ce69a7a603c00f9a3e56479e912b

I have also simplified the vsgimgui_examnple by changing it so the MyGui class simply inherits from vsg::Commnad and overrides the compile(Context&) in order to compile the texture and record(CommandBuffer&) to record the ImGui calls. This creates a self contained class for doing the ImGui calls with the constructor doing the texture set up rather than passing in a ImageComponent. The change for this is:

https://github.com/vsg-dev/vsgExamples/commit/cef4072cbb7447d1a23c0d77f66378b2f8d5192c

[robertosfield]

I have just completed my final refactoring of the ImGuiNode branch with the renaming of vsgImGui::ImageComponent to vsgImGui::Texture to keep it inline with ImGui terminology better. This refactor work is now ready to merge with master so I've updated the vsgImGui version to 0.1.0 and created a PR for it: #47 so will close this PR as all the functionality is part of 47.

[timoore]

It looks good here. I've replaced my ImageComponent with vsgImGui::Texture in vsgCs now, and it all works. For a slight variation on the example, I'm creating the Texture objects asynchronously from the initial creation of the ImGui interface.

I like the use of vsg::Command to support texture compilation even though a Texture isn't a command, and the record() method for running the component even if it isn't recording anything in a command buffer :smile: I did a similar thing with a CompilableImage in vsgCs.

I'll start submitting some of my other derived VSG classes in order to get my line count down...

[vsg-dev]

That's a relief, I was hoping that things would just work for you.

W.r.t use of vsg::Command, perhaps another base class vsg::Compilable may be useful base c;lass to have for objects that need to be compiled by never have any Command associated with them. The VSG has to small CPU overhead when encountering Command during the ReocrdTraversal - the vsg::State has to be made current before the command::record(..) is called and this will happen even when a Command implementation is a non op.

[robertosfield]

I've just looked into the possibility of creating a Compilable base class and realized that for the current usage vsgImGui::Texture would work fine just subclassing from vsg::Object and having the compile called explicitly. The following mod to vsgImGui works fine with the vsgimage_example without any changes to the example as it explicitly calls texture->compile().

.$ git diff diff --git a/include/vsgImGui/Texture.h b/include/vsgImGui/Texture.h index 11f0996..02816f1 100644 --- a/include/vsgImGui/Texture.h +++ b/include/vsgImGui/Texture.h @@ -35,13 +35,12 @@ SOFTWARE. namespace vsgImGui { /// Texture adapter that uses a DescriptorSet/DescriptorImage to hold a texture image on the GPU in a form that ImGui can use.

• class VSGIMGUI_DECLSPEC Texture : public vsg::Inherit<vsg::Command, Texture>

• class VSGIMGUI_DECLSPEC Texture : public vsg::Inherit<vsg::Object, Texture> { public: Texture(vsg::ref_ptr data = {});

• void compile(vsg::Context& context) override;

• void record(vsg::CommandBuffer& commandBuffer) const override;

• void compile(vsg::Context& context);

  /// get the ImTextureID used with ImGui::Image(..) calls
  ImTextureID id(uint32_t deviceID) const;

  diff --git a/src/vsgImGui/Texture.cpp b/src/vsgImGui/Texture.cpp index b444acb..e9733f3 100644 --- a/src/vsgImGui/Texture.cpp +++ b/src/vsgImGui/Texture.cpp @@ -77,8 +77,3 @@ ImTextureID Texture::id(uint32_t deviceID) const { return descriptorSet ? static_cast(descriptorSet->vk(deviceID)) : ImTextureID{}; }

• -void Texture::record(vsg::CommandBuffer&) const -{

• // non op -}

[timoore]

That change would be a little less convenient for vsgCs because I use the viewer's CompileManager to compile the Texture, which IIUC would invoke the compile() virtual function as part of a traversal.

[robertosfield]

On Wed, 22 Mar 2023 at 18:09, Tim Moore @.***> wrote:

That change would be a little less convenient for vsgCs because I use the viewer's CompileManager to compile the Texture, which IIUC would invoke the compile() virtual function as part of a traversal.

If you subclass from vsg::Command and override compile() like I've done with the vsgimgui_example it should be invoked in the CompileTraversal, if you just assigned the vsgImGui::Texture as it's own object in the scene graph or call compile on it directly then it'll need to remain subclassed from vsg::Command, or a new vsg:::Compileable base class.

Have you checked in your changes to vsgCs to use the new vsgImGui::Texture support? Tomorrow I could have a look at the usage case and reflect on what changes might make things cleaner.

Message ID: @.***>

[timoore]

On Wed, 22 Mar 2023 at 18:09, Tim Moore @.***> wrote: That change would be a little less convenient for vsgCs because I use the viewer's CompileManager to compile the Texture, which IIUC would invoke the compile() virtual function as part of a traversal. If you subclass from vsg::Command and override compile() like I've done with the vsgimgui_example it should be invoked in the CompileTraversal, if you just assigned the vsgImGui::Texture as it's own object in the scene graph or call compile on it directly then it'll need to remain subclassed from vsg::Command, or a new vsg:::Compileable base class. Have you checked in your changes to vsgCs to use the new vsgImGui::Texture support? Tomorrow I could have a look at the usage case and reflect on what changes might make things cleaner.

Just to be clear, the way Texture is defined now is fine for me. Whether a compilable thing is a subclass of Command or something called Compilable is really just a detail that can be resolved much later IMHO.

In my usage of ImGui / Texture, I can't just compile it when the "component" is compiled, because I don't have the image yet; it's loaded later via http when the Cesium credits system detects that ion needs to be credited.

My use of vsgImGui::Texture in vsgCs is all checked in.