[VL.ImGui.Stride.Viewports] #673

kopffarben · 2024-04-16T11:24:49Z

PR Details

Viewports support for VL.Imgui.Stride

Description

This is an attempt to support ImGui viewports in VL.

It already works quite well, there are a few small things that do not work satisfactorily yet.

IssueTracker: https://github.com/kopffarben/VL.StandardLibs/issues

The previous features should all still work. Not 100 percent sure.

The problem with the Skia input should also be fixed.
I think Kairos is still working, but I'm not quite sure as I don't know exactly how it should behave.

In general, it could be cleaned up again, I'm also not completely satisfied with the naming of some classes ... but in principle it's good for now.
I'm also not quite sure if this is the best solution for some things.

I would be very happy about a code review.

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)

compiler Error after Cloning in ImGuiEffect.sdfx.cs

VL.ImGui.Stride was not updated in VL when edit the source problem was different TargetFramework in csproj (net8.0-windows) and vvvv\packs\VL.ImGui.Stride\lib (net8.0-windows7.0) so vl use allways version from vvvv-folder

…Gui.Stride.InputSource

remove InputManager from RenderLayerWithViewPort and get resourceHandle in MappedInputSource

…Gui.Stride.InputSource

now inputSource works from beginning See MappedInputSource.Dispose // TODO ... this will lead into 'Access violation' //inputManager.Sources.Remove(this);

CleanUp more GC friendly (reuse PointerPoint if possible)

…Gui.Stride.Viewports

known issues: InputHandling need to be checked Didn't work after restart F9 some bugs need to be found

…Gui.Stride.Viewports

maybe fixed SkiaWidget

Remove unused RenderView from RenderWidget ... is now done in vl ClenUp

azeno

Thanks for this huge effort and sorry that it took a while to give it a spin. I left some comments in code.

Was testing a little bit with your demo patch and once you start moving windows out and back in again the input handling seems to fall apart at some point. From the help patch drag out (to the right) the StrideSceneWithInput, it still works. Now move it back in, it stops interacting.

azeno · 2024-05-22T16:20:40Z

VL.ImGui.Skia/src/SkiaWidget.cs

@@ -60,13 +63,14 @@ protected override void Draw(Context context, in ImDrawListPtr drawList, in Syst
        public RectangleF? Bounds => !_disposed ? Layer?.Bounds : default;

        // What is this for?
-        //SKMatrix? trans;
+        // it is somehow necessary for the Stride/Skia inputHandleing to work ... is a strange workaround


Hm ok, can you point to a patch where these lines of code do make a difference?

Because the CallerInfo in Notify has the wrong transformation ... The CallerInfo in Render has the correct ... just set a breakpoint in Notify ... then you can see it

Interesting - so this might be a bug in the Stride-Skia bridge then?

Yes, probably that

Will have a look..

Just pushed a fix for the SkiaRenderer - like you say, it was indeed broken, as one could see with the Stride/Skia overlay help patch. Using a Skia Button there didn't work. Thanks for the pointer. You should be able to remove that above hack now.

azeno · 2024-05-22T16:57:40Z

VL.ImGui/src/Helper/RenderHelper.cs

-                // The up & down event methods don't take the position as an argument. Therefor make sure it's present, or we end up with wrong clicks when using touch devices.
-                var pos = useWorldSpace ? mouseNotification.PositionInWorldSpace.FromHectoToImGui() : mouseNotification.Position.ToImGui();
-                _io.AddMousePosEvent(pos.X, pos.Y);
+                var flag = ImGui.GetIO().ConfigFlags;


Do you remember why this change was necessary?

If viewports are enabled, ImGui expects the mouse position in DesktopSpace/ScreenSpace and no longer in WindowSpace

I derived this from the viewport example from ImGui.net

Ok but the code comment says it's about the position on touch down events - not about spaces.

That was an old code comment from you ... maybe I should have put it out there
I should clean it up again

I know I wrote it. The comment says that the MouseMove is necessary to allow TouchDown events to be on the right spot, because the down event itself doesn't take a position. You surrounded the whole part in an if statement, which introduces different behavior. You say it was necessary because ImGui expects the positions to be in screen space, in which case I'd argue that we need to feed the right coordinates to the move event, but not skip it entirely.

Imgui, in viewport mode, expects a mouse position in the ScreenSpace for each frame. This is set in ImGuiWindows.cs SetPerFrameImGuiData().

I can therefore also put it in an If at this point, as it is set anyway

Aha, that solves the puzzle. Then either add that piece of info to the if statement or get rid of it entirely and set the mouse position in each frame in existing mode.

azeno · 2024-05-22T16:58:46Z

VL.Stride.Runtime/src/Games/GameWindowRenderer.cs

@@ -75,6 +75,11 @@ public override void Initialize()
            base.Initialize();
        }

+        public void Close()


Calling Dispose was not an option?

I'm no longer sure about this ... I think it had a reason ... my Close just calls Destroy ... just tested briefly ... Dispose also works at least ... I'll have to take a closer look again

azeno · 2024-05-22T17:00:27Z

VL.ImGui.Stride/src/VL.ImGui.Stride.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFramework>net8.0-windows</TargetFramework>
+    <TargetFramework>net8.0-windows7.0</TargetFramework>


Again this guy ;) Still not sure about it, we have many other projects using net8.0-windows which do not need it.

Also works without ... is probably a leftover

kopffarben · 2024-05-23T11:01:30Z

Because the window out and in again bug, i.e. losing the input handling, I will also take another close look at it. I'm just relatively busy at the moment. But hopefully I'll get back to it tonight or in the next few days. I think it also needs another cleanup

…its Notify calls (came up in #673)

… path for notifications as normal Skia rendering uses (issue vvvv#672)

azeno · 2024-05-23T13:44:40Z

@kopffarben My last commit (linked above) seems to have fixed #672 as well. However there was also that boolean flag I had to add as a workaround back in April which I now managed to get rid of by using the same code path for notifications as the normal Skia render path takes (see linked commit in #672, 2b92219). I didn't merge it back to main yet because of our discussion further up which is all about that line. You said in docking branch it expects the screen space coordinates in AddMousePosEvent - but then I wonder how this will work together with current approach / approach taken in my new branch where we feed the PositionInWorldSpace which to my understanding takes those Skia viewports into account.

Some Comments

…Gui.Stride.Viewports

remove transformation workaround ... not longer needed ... fixed with 8e87a5a

kopffarben · 2024-05-23T15:15:11Z

the AddMousePosEvent is only set if ImGui is not in viewport mode. If ImGui is in viewport mode, the screen space mouse position is set every frame in ImGuiWindows.

private void SetPerFrameImGuiData()
{
    unsafe
    { 
        int x, y;
        uint buttons = global::Stride.Graphics.SDL.Window.SDL.GetGlobalMouseState(&x, &y);
        _strideDeviceContext.IO.MouseDown[0] = (buttons & 0b0001) != 0;
        _strideDeviceContext.IO.MouseDown[1] = (buttons & 0b0010) != 0;
        _strideDeviceContext.IO.MouseDown[2] = (buttons & 0b0100) != 0;
        _strideDeviceContext.IO.MousePos = new Vector2(x, y);
    }

    _strideDeviceContext.IO.DisplaySize = new Vector2(mainViewportWindow.Size.X, mainViewportWindow.Size.Y);
    _strideDeviceContext.IO.DisplayFramebufferScale = new Vector2(1.0f, 1.0f);
    
    
    ImGui.GetPlatformIO().Viewports[0].Pos = new Vector2(mainViewportWindow.Position.X, mainViewportWindow.Position.Y);    
    ImGui.GetPlatformIO().Viewports[0].Size = new Vector2(mainViewportWindow.Size.X, mainViewportWindow.Size.Y);
}

So it should not break the current approach

…eature/VL.ImGui.Stride.Viewports

kopffarben · 2024-05-23T16:51:50Z

SO I fixed most of it for now, merged main and the bugfix. The problem with pulling the window in and out still exists.

I lose the input source or the appropriate viewport after dragging in and out a few times. Sometimes it goes faster ... but I don't have enough time right now to investigate it more closely. But I will keep at it.

use VL.ImGui.Stride/src/ImGuiRenderer.cs from kopffarben

known issue ... remove IInputSource from normal Stride Windows

…r.cs Use this Option in WindowCore to has an UniqueName, so save WindowLayout now working remove temp loggiong ... was for debugging

…Gui.Stride.Viewports.WIP

…reen was wron ... width was 0 ... bug in Stride

…Gui.Stride.Viewports.WIP

Refactored `ImGuiWindow` and `ImGuiWindows` classes to use `IResourceHandle<InputManager>` and handle null `Window` objects gracefully. Updated `Dispose` methods across multiple classes to follow the standard dispose pattern, including adding `Dispose(bool disposing)` methods and finalizers. Removed `InputManagerExtension` class. Improved code readability with minor formatting changes.

Introduce UniqueId field to RenderWidget for unique naming. Add constructors to initialize UniqueId. Remove nodeContext property. Update ImGui.BeginChild to use UniqueId for unique names.

- Added new input pins ("Bounds," "Save Bounds," "Enable Camera," "EditMode") and removed some ("Edit Mode," "Node Context") in `VL.ImGui.Stride.vl` to improve functionality. - Modified `HowTo ImGui Windows Docking.vl` - Added `GameWindow` property and modified constructors in `ImGuiWindow.cs` and `ImGuiWindows.cs` to accept `RectangleF bounds`.

…Gui.Stride.Viewports

kopffarben · 2024-12-18T17:29:58Z

SO finally I have it ... Yeahhh

I also had to use the SourceGenerator to get the layout saving of ImGui to work. Does nothing else than bringing the NodeContext in the Window Widget into the Constructor ... so I have a UniqueID for the names of the windows that I can add to the name name##uniqueID

The InputHandling was a really difficult matter ... but now it works completely ... I always capture the correct InputSource ... and the mapping to the SubInputSources always works.

Apart from that, I also handle the commands, perfmeters and so on ... Even the EditMode works, oh yes, I also save the bounds of the main window in the InputPin ... Copy from RenderWindow

Just take a look at Helppatch ... it's almost a full-blown application ... Windows, save layout, edit layout ... , LightMode/DarkMode ... etc.

kopffarben added 30 commits March 12, 2024 13:41

BUGFIX:

09bf3cc

compiler Error after Cloning in ImGuiEffect.sdfx.cs

BUGFIX:

9ee818e

VL.ImGui.Stride was not updated in VL when edit the source problem was different TargetFramework in csproj (net8.0-windows) and vvvv\packs\VL.ImGui.Stride\lib (net8.0-windows7.0) so vl use allways version from vvvv-folder

working SceneWidget and remove Properties/launchSettings.json

482e798

WIP RenderWidget with IInputSource Out

93ec848

WIP

3e251e6

WIP CleanUp

a065910

WIP

aede9c9

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

e423c20

…Gui.Stride.InputSource

WIP

fdc69fb

remove InputManager from RenderLayerWithViewPort and get resourceHandle in MappedInputSource

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

1318fee

…Gui.Stride.InputSource

WIP

4d30715

now inputSource works from beginning See MappedInputSource.Dispose // TODO ... this will lead into 'Access violation' //inputManager.Sources.Remove(this);

WIP

e53e513

WIP

75dffb6

CleanUp more GC friendly (reuse PointerPoint if possible)

WIP

74d07a3

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

04d9e0d

…Gui.Stride.Viewports

WIP

53688ac

CleanUP

163e258

remove InputManager from StrideContext ... not needed

9edf8bd

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

f28bf90

…Gui.Stride.Viewports

Move Stride Pipeline Stuff to StrideDeviceContext

893137c

CleanUp

777c985

WIP

4a57a14

WIP

d2410ea

WIP first buggy working version ...

347b9f3

Working Version for Viewports

7011c91

known issues: InputHandling need to be checked Didn't work after restart F9 some bugs need to be found

some CleanUp

0f363eb

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

4cfd87a

…Gui.Stride.Viewports

SkiaRendererWithOffset

e27cb98

maybe fixed SkiaWidget

WIP

9e05b53

ADD Bound to Document

060c2d2

Remove unused RenderView from RenderWidget ... is now done in vl ClenUp

azeno reviewed May 22, 2024

View reviewed changes

azeno added a commit that referenced this pull request May 23, 2024

Fixes SkiaRenderer not propagating the right transformation during …

8e87a5a

…its Notify calls (came up in #673)

[ImGui.Stride] Gets rid of earlier introduced hack by using same code…

2b92219

… path for notifications as normal Skia rendering uses (issue vvvv#672)

azeno mentioned this pull request May 23, 2024

Mouse positions don't work in Stride -> ImGui -> Skia setup #672

Closed

kopffarben added 3 commits May 23, 2024 15:57

VL.ImGui.Stride TargetFramework net8.0-windows

679e1f8

Some Comments

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

6f46662

…Gui.Stride.Viewports

[VL.ImGui.Skia]

bdafce1

remove transformation workaround ... not longer needed ... fixed with 8e87a5a

Merge remote-tracking branch 'remotes/origin/bugfix/issue-672' into f…

cec33a3

…eature/VL.ImGui.Stride.Viewports

kopffarben added 17 commits May 30, 2024 16:13

IMappedDevice add SetSourceDevice

524c1a8

Merge branch 'main' into feature/VL.ImGui.Stride.Viewports

71e475a

use VL.ImGui.Stride/src/ImGuiRenderer.cs from kopffarben

small bug-fix

a71b894

BugFix InputManager.Remove ... breaks after a while

aa800be

known issue ... remove IInputSource from normal Stride Windows

WIP

0d538d9

Add Option for Constructor(NodeContext nodeContext) to SourceGenerato…

3c8c945

…r.cs Use this Option in WindowCore to has an UniqueName, so save WindowLayout now working remove temp loggiong ... was for debugging

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

72f9f29

…Gui.Stride.Viewports.WIP

fix move to second screen ... DesktopBounds from Stride for second Sc…

6d3f50d

…reen was wron ... width was 0 ... bug in Stride

revert launchSettings.json

085b92a

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

c171198

…Gui.Stride.Viewports.WIP

so improvements in ImGuiWindows.cs

fda0109

Add UniqueId to RenderWidget for unique naming

a461a82

Introduce UniqueId field to RenderWidget for unique naming. Add constructors to initialize UniqueId. Remove nodeContext property. Update ImGui.BeginChild to use UniqueId for unique names.

Nicer Layout

2197d3e

Merge remote-tracking branch 'remotes/origin/main' into feature/VL.Im…

5311019

…Gui.Stride.Viewports

Work on Helppatch

ddfc695

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VL.ImGui.Stride.Viewports] #673

[VL.ImGui.Stride.Viewports] #673

kopffarben commented Apr 16, 2024

azeno left a comment •

edited

Loading

azeno May 22, 2024

kopffarben May 23, 2024

azeno May 23, 2024

kopffarben May 23, 2024

azeno May 23, 2024

azeno May 23, 2024

azeno May 22, 2024

kopffarben May 23, 2024

azeno May 23, 2024

kopffarben May 23, 2024

azeno May 23, 2024

kopffarben May 23, 2024

azeno May 23, 2024

azeno May 22, 2024

kopffarben May 23, 2024

azeno May 22, 2024

kopffarben May 23, 2024

kopffarben commented May 23, 2024

azeno commented May 23, 2024

kopffarben commented May 23, 2024

kopffarben commented May 23, 2024

kopffarben commented Dec 18, 2024

[VL.ImGui.Stride.Viewports] #673

Are you sure you want to change the base?

[VL.ImGui.Stride.Viewports] #673

Conversation

kopffarben commented Apr 16, 2024

PR Details

Description

Types of changes

azeno left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kopffarben commented May 23, 2024

azeno commented May 23, 2024

kopffarben commented May 23, 2024

kopffarben commented May 23, 2024

kopffarben commented Dec 18, 2024

azeno left a comment •

edited

Loading