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

CaptureScreenshotWindow("...", ImGuiCaptureFlags_StitchAll ) fails #33

Open
pthom opened this issue Sep 28, 2023 · 9 comments
Open

CaptureScreenshotWindow("...", ImGuiCaptureFlags_StitchAll ) fails #33

pthom opened this issue Sep 28, 2023 · 9 comments
Labels

Comments

@pthom
Copy link
Contributor

pthom commented Sep 28, 2023

Re-Bonjour Omar,

Sorry if I'm flooding the issues a bit today.

I'm trying the code of app_minimal, and I have an issue with screen capture and the flag ImGuiCaptureFlags_StitchAll.

Basically,

  • I'm getting a fully black capture under windows (although it has the correct size). This is strange. I'm using Windows 10 ARM 64 bits (although I compiled the code in x64 mode)
  • under macOS (after having applied a hack to take into account the FrameBufferScale), the scroll does not work, and I get a repeated image:
image

i.e. the size is correct, but it seems like the content is repeated (as if the scroll did not work).
Based on the usleep(1000) for linux, I tried to add a sleep(1 second) during the capture, and I could see that effectively the scroll does not work.

@ocornut
Copy link
Owner

ocornut commented Sep 28, 2023

I tested it and it seems to be working for me (Windows 11, x64).

I have however now confirmed that it breaks with io.ConfigWindowsMoveFromTitleBarOnly = true in the same way as your repeated capture. Seems fixable.

(Initially we designed/created the capture tool to work completely outside of Test Engine, which required some hoops and design choices which i think may be irrelevant today. It may be a good idea to rewrite it and embrace using test engine.)

As for one failing under Windows: which backend you are using?
You my add breakpoint in imgui_app.cpp's ImGuiApp_ScreenCaptureFunc() to investigate further.

@pthom
Copy link
Contributor Author

pthom commented Sep 28, 2023

I have however now confirmed that it breaks with io.ConfigWindowsMoveFromTitleBarOnly = true in the same way as your repeated capture. Seems fixable.

Good catch! I if set io.ConfigWindowsMoveFromTitleBarOnly = false, then it works
image

I was getting this issue from inside HelloImGui, where io.ConfigWindowsMoveFromTitleBarOnly is false by default. I'll soon post an issue inside HelloImGui and give some details about how it is going. If you want to give me some output it would be appreciable!

Now, getting on to debug under windows

@pthom
Copy link
Contributor Author

pthom commented Sep 28, 2023

Under windows, I have found the issue: I'm using the docking branch, and this code inside app_minimal_main.cpp is the reason it fails:

#ifdef IMGUI_HAS_VIEWPORT
    io.ConfigFlags |= ImGuiConfigFlags_ViewportsEnable;
#endif

If I comment out this line, then it works! I think your implementation mentions somewhere that viewports are not handled for screen capture.

ocornut added a commit that referenced this issue Sep 28, 2023
….ConfigWindowsMoveFromTitleBarOnly option is set. (#33)
ocornut added a commit that referenced this issue Sep 28, 2023
Was originally aimed at solving #33 before I realized even calling SetWindowPos() every frame wouldn't work.
Minor refactor however seems decent and goes in the direction of a more stateless rework.
ocornut added a commit that referenced this issue Sep 28, 2023
…iewport. (#33)

Needed to retire IMGUI_APP_IMPLEMENTATION from imgui_app.cpp as it needs to access backend internals.
StitchAll still broken.
@ocornut
Copy link
Owner

ocornut commented Sep 28, 2023

  • I fixed the issue with io.ConfigWindowsMoveFromTitleBarOnly and stitching.

  • ImGuiCaptureFlags_StitchAll doesn't currently work with multi-viewport because the way we move the window makes it move to its own viewport whereas we try to capture from the main viewport. I however pushed changes to support capturing from secondary viewport in imgui_app (the API was already there but not supported by imgui_app), but it requires further changes in capture tool too.

@pthom
Copy link
Contributor Author

pthom commented Sep 28, 2023

I fixed the issue with io.ConfigWindowsMoveFromTitleBarOnly and stitching

I confirm that your fix works

ocornut added a commit that referenced this issue Sep 28, 2023
…iewport. (#33)

Needed to retire IMGUI_APP_IMPLEMENTATION from imgui_app.cpp as it needs to access backend internals.
StitchAll still broken.
@ocornut
Copy link
Owner

ocornut commented Sep 28, 2023

I put a little more effort on trying to fix various things with the capture tool, but I believe the codebase is too flawed, it may be simpler to rewrite with specialized/separate function for single capture, stitched capture, video capture etc. and test support for viewport and DPI shenanigans.

@pthom
Copy link
Contributor Author

pthom commented Sep 28, 2023

It happens to all of us :-)

May be adding simple structs to store a capture and its coords such as below could help to make the API a bit simpler.

struct ImageBufferRgba
{
    int Width = 0, Height = 0;
    ImVector<uint8_t> Buffer;
};

struct CaptureCoords
{
   ImGuiID ViewportId;
   int x, int y, int w, int h; // Or maybe ImRect
};

with the added benefits that:

  • transforming CaptureCoords into new coords taking into account FrameBufferScale now becomes easier
  • resizing ImageBufferRgba.Buffer can be done when needed (i.e. at the moment of the capture). Signalling that the image buffer size differs from the window size becomes also possible.

And the callback signature could then become:

typedef bool (ImGuiScreenCaptureFunc)(CaptureCoords coords, ImageBufferRgba* buffer, void* user_data);

However, this would probably change too much user code for those who already implemented a callback.

@pthom
Copy link
Contributor Author

pthom commented Sep 28, 2023

... and there are probably lots of other reasons for which this would not work ;-)

Sorry if my suggestion was inappropriate, esp given that ImGuiCaptureImageBuf already exists.

@pthom
Copy link
Contributor Author

pthom commented Sep 28, 2023

FYI, I started to work on integrating imgui_test_engine into hello_imgui (which is a preliminary, before integrating into Dear ImGui Bundle).

If you are interested, I made a quick demo on how easy it can be to setup a test app here:
https://github.com/pthom/hello_imgui_test_engine_demo

At the moment, the integration of imgui_test_engine is not in the main branch of HelloImGui, but in the with_imgui_test_engine branch.

I'll make sure to add some doc about its license when I release it in the main branch.

The code for integratings the test engine was quite simple to add, and is grouped in this folder.

@ocornut ocornut added the capture label Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants