[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

…Gui.Stride.Viewports

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.

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

kopffarben added 6 commits April 16, 2024 13:04

revert launchSettings.json

cd90ed0

Closing MainWindow will close all other Windows

9d77ae2

Add MousButtons to globalMouseState

c78d9ab

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

60e4025

…Gui.Stride.Viewports

WIP fixed null ref on Closeing

d9d4a7a

fix InputSource Handeling

57254be

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

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

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

[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

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

azeno left a comment •

edited