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

Feature/VL.ImGui.Stride #667

Merged
merged 25 commits into from
Mar 7, 2024
Merged

Conversation

kopffarben
Copy link
Contributor

PR Details

ImGui.Renderer for Stride

Description

VL.ImGui.Stride.Renderer works exactly like the Skia version (ImGui Region), it only renders the buffers in Stride.

Known problems:

  • Colors don't match, I think it has to do with the ColorSpace. should be fixable in the shader though. Currently I'm trying SRgbToLinearPrecise() in ImGuiShader_DrawFX.sdsl is better but doesn't fit exactly.

  • SkiaLayerWidget does not work.

  • If you change the font, the old font is not disposed. which leads to a memory leak if you try a lot of fonts. possibly an ImGui.Net problem. Problem already existed before.

    • var f = atlas.AddFontFromFileTTF(path, cfg.SizePixels, &cfg, GetGlypthRange(atlas, font.GlyphRange));

Motivation and Context

Missing so far

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)

Checklist

I moved a couple of functions that are the same in Skia and Stride to VL.ImGui.RenderHelper to have a common codebase.

  • INotification Handeling and ButtonMapping
  • FontAtlasBuilding
  • Scaling

known issues:
- looks different from SKIA Version
  - wrong ColorSpace?
  - wrong Style?
- skia LayerWidget not working
…t used by ToSkiaLayer and ImGuiRenderer

so one CodeBase for INotifiction Handling (KeyMapping), FontAtlasBuilder, and Scaling
@azeno
Copy link
Member

azeno commented Feb 14, 2024

Great work! Thank you!

Regarding wrong colors:
On the CPU side our colors (like stored in pins and IO boxes) are in gamma space. When feeding them to the GPU we need to convert them to the device color space. For that we should add a virtual method virtual System.Numerics.Vector4 ToDeviceColorSpace(Color4) on the internal Context class, which both the Skia and Stride backends can override as needed. We then need to identify where colors get fed into the ImGui API and call that method accordingly beforehand. Once we identified all those places we can be sure that the vertex buffers generated by ImGui will contain colors in the proper color space. No need now to do anything special in the shader.

I'll try to put those thoughts as comments in your changes.

Copy link
Member

@azeno azeno left a comment

Choose a reason for hiding this comment

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

By adding virtual System.Numerics.Vector4 ToDeviceColorSpace(Color4 color) to Context and overriding it accordingly in SkiaContext and StrideContext we should be able to replace all current calls to https://github.com/vvvv/VL.StandardLibs/blob/main/VL.ImGui/src/Core/ImGuiUtils.cs#L289 with it.

I guess we also will need to take care of setting up the default style properly. Will need to have a chat with @antongit about it how this is setup currently.


imShader = drawEffect;

_context = new Context();
Copy link
Member

Choose a reason for hiding this comment

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

This would become new StrideContext(device) with StrideContext overriding the newly introduced ToDeviceColorSpace method which in turn needs to be called by all parts of VL.ImGui which feed colors to ImGui.

Copy link
Contributor Author

@kopffarben kopffarben Feb 14, 2024

Choose a reason for hiding this comment

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

I tried this with the StrideContext.ToDeviceColorSpace(...).

    internal sealed class StrideContext : Context
    {
        public override System.Numerics.Vector4 ToDeviceColorSpace(Color4 color)
        {
            return base.ToDeviceColorSpace(color.ToLinear());
        }
    }

only to find out that the colors from the style do not fit
then I converted all colors in the style as follows.

var s = ImGui.GetStyle();
for (int i = 0; i < s.Colors.Count; i++)
{
    var color = Unsafe.As<System.Numerics.Vector4, Color4>(ref s.Colors[i]).ToLinear();
    s.Colors[i] = Unsafe.As<Color4, System.Numerics.Vector4>(ref color);
}

to then realize that it had exactly the same result as the shader.

looks almost like the Skia part but only almost. is brighter

Haven't pushed these changes yet

Copy link
Member

Choose a reason for hiding this comment

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

Where is this style stuff happening?

Copy link
Member

Choose a reason for hiding this comment

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

And I think the implementation should be

return device.ColorSpace == ColorSpace.Linear ? color.ToLinear() : color;

VL.ImGui.Stride/src/ImGuiRenderer.cs Outdated Show resolved Hide resolved
VL.ImGui.Stride/src/ImGuiRenderer.cs Outdated Show resolved Hide resolved
VL.ImGui.Stride/src/ImGuiRenderer.cs Outdated Show resolved Hide resolved
VL.ImGui.Stride/src/ImGuiRenderer.cs Outdated Show resolved Hide resolved
VL.ImGui.Stride/src/VL.ImGui.Stride.csproj Outdated Show resolved Hide resolved
VL.ImGui.Stride/src/ImGuiRenderer.FontTexture.cs Outdated Show resolved Hide resolved
@azeno azeno added enhancement New feature or request VL.Stride Wrapper for 3d game engine Stride labels Feb 14, 2024
@azeno
Copy link
Member

azeno commented Feb 14, 2024

SkiaLayerWidget does not work. That's ok for now. If we figure out a way to connect the existing bridge with it - cool. But would see this is a separate task.
What should be doable is a RenderWidget, but again, it's ok if it comes in later.

use IResourceHandle where it is needed
set VLPackageBasePath.Description
add StrideContext.cs
@azeno
Copy link
Member

azeno commented Feb 16, 2024

Just tested your branch myself and it seems my solution also doesn't work - for example the color picker will look completely wrong. Guess it needs to be solved in the shader like you tried initially. See ocornut/imgui#578 (comment)

Some CleanUp
First Try of RenderWidget
- Depht isn't correct ...
- seems we need render to Texture
ADD ClipRect to Windows/WindowCore.cs and Windows/ChildWindow.cs
Now use drawList.AddCallback to pass ILayer from SkiaWidget to ToSkiaLayer
SkiaContextet is now used just for Notification
So get rid off spooky low IntPtr workaround
@kopffarben
Copy link
Contributor Author

So I found some time to continue working here.

I got quite far. Helppatches and so on have to be made pretty again.

Apart from that I now have a TextureWidget, a RenderWidget and the LayerWidget also works.

I also had to adjust ToSkiaLayer.cs, SkiaWidget.cs and SkiaContext.cs a bit to get the LayerWidget working.

I changed the somewhat strange workaround with the small texture IDs.
Now use drawList.AddCallback to pass ILayer from SkiaWidget to ToSkiaLayer
SkiaContextet is now used just for Notification
So get rid off spooky low IntPtr workaround

known issus:
the notification in the layer widget does not work yet,

@kopffarben
Copy link
Contributor Author

Would be great if you could do a code review

@azeno
Copy link
Member

azeno commented Mar 6, 2024

Will have a look at this today. Is it ready to get merged from your end?

@kopffarben
Copy link
Contributor Author

Most Thinks working and well testet. EntityWidget is WIP ... and Helppatches need to be touched.

@azeno azeno self-requested a review March 7, 2024 11:11
Copy link
Member

@azeno azeno left a comment

Choose a reason for hiding this comment

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

Crazy that Skia -> Imgui -> Stride works. I wasted hours yesterday on the GammaToLinear topic and found no satisfying solution. I guess we'll have to live with it.

Thank you for all that work. I'll merge this PR now and we deal with any remaining issues from there.

@azeno azeno merged commit 7899b16 into vvvv:main Mar 7, 2024
1 check passed
@azeno azeno mentioned this pull request Mar 7, 2024
azeno added a commit that referenced this pull request Apr 10, 2024
…667 (7899b16)

This commit breaks the SkiaStrideInputHandling help patch, but existing patches like Kairos Compositor work again. For now this is the better trade off.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request VL.Stride Wrapper for 3d game engine Stride
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants