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

Backends: DX9: programmable rendering pipeline version implement (Dear @Demonese) #3844

Closed
wants to merge 6 commits into from

Conversation

Xiliusha
Copy link

@Xiliusha Xiliusha commented Feb 26, 2021

1. BGRA color packing (also fix a bug)

DX9 using B8G8R8A8 color packing (D3DCOLOR). I notice that imgui_impl_dx9 do color packing converting but in core library there is a IMGUI_USE_BGRA_PACKED_COLOR macro.
So I think we can avoid color packing converting:

  • before:
vtx_dst->col = (vtx_src->col & 0xFF00FF00) | ((vtx_src->col & 0xFF0000) >> 16) | ((vtx_src->col & 0xFF) << 16);     // RGBA --> ARGB for DirectX9
  • after:
#ifndef IMGUI_USE_BGRA_PACKED_COLOR
    vtx_dst->col = (vtx_src->col & 0xFF00FF00) | ((vtx_src->col & 0xFF0000) >> 16) | ((vtx_src->col & 0xFF) << 16);     // RGBA --> ARGB for DirectX9
#else
    vtx_dst->col = vtx_src->col;
#endif

This change will also fix a bug (default dark theme):

  • before:
    image
  • after:
    image

2. Bug fix

I think we forget to validate device and texture before drawing. My game crashes in some cases because of this.

// Render function.
void ImGui_ImplDX9_RenderDrawData(ImDrawData* draw_data)
{
    // Avoid rendering when device or texture is invalid
    if (!g_pd3dDevice || !g_FontTexture)
        return;

3. The ordering of state recovery

I don't known why, but this did fix a bug in my game.
(see commits)

4. Static texture

We can create static texture to improve performance.

// Create a texture without D3DUSAGE_DYNAMIC (static texture)
// https://github.com/Microsoft/DirectXTex

// 1. Create staging texture and write pixel data
IDirect3DTexture9* texture_staging = NULL;
struct com_helper
{
    // This is a helper to manage COM object
    // We can also using Microsoft::WRL::ComPtr (wrl.h) or CComPtr (ATL)
    IDirect3DTexture9*& ptr;
    com_helper(IDirect3DTexture9*& tex) : ptr(tex) {}
    ~com_helper() { if (ptr) { ptr->Release(); ptr = NULL; } }
} texture_staging_ref(texture_staging);
if (D3D_OK != g_pd3dDevice->CreateTexture(width, height, 1, D3DUSAGE_DYNAMIC, D3DFMT_A8R8G8B8, D3DPOOL_SYSTEMMEM, &texture_staging, NULL))
    return false;
D3DLOCKED_RECT tex_locked_rect;
if (D3D_OK != texture_staging->LockRect(0, &tex_locked_rect, NULL, 0))
    return false;
for (int y = 0; y < height; y++)
    memcpy((unsigned char*)tex_locked_rect.pBits + tex_locked_rect.Pitch * y, pixels + (width * bytes_per_pixel) * y, (width * bytes_per_pixel));
if (D3D_OK != texture_staging->UnlockRect(0))
    return false;

// 2. Upload to graphic card memory
g_FontTexture = NULL;
if (D3D_OK != g_pd3dDevice->CreateTexture(width, height, 1, 0, D3DFMT_A8R8G8B8, D3DPOOL_DEFAULT, &g_FontTexture, NULL))
    return false;
if (D3D_OK != g_pd3dDevice->UpdateTexture(texture_staging, g_FontTexture))
    return false;

Thanks Dear-ImGui

@Xiliusha Xiliusha changed the title Backends: DX9: add missing IMGUI_USE_BGRA_PACKED_COLOR support Backends: DX9: several improvements Feb 26, 2021
@metarutaiga
Copy link

metarutaiga commented Feb 26, 2021

1. BGRA color packing

It would be poor performance to copy vertex color to graphic memory.
Edit: Sorry, I made a mistake, I read wrong parts.

4. Static texture

It's the same problem as BGRA color packing, here is D3DFMT_A8R8G8B8, so you need the IMGUI_USE_BGRA_PACKED_COLOR.
And it can't be support Color Emoji Fonts now.

To create a staging texture to upload texture is necessary, it can change the linear texture to optimal texture (tiling texture).
I found all backends, the D3D9 backend is missing the feature.

@Xiliusha
Copy link
Author

Xiliusha commented Feb 26, 2021

1. BGRA color packing

It would be poor performance to copy vertex color to graphic memory.

4. Static texture

It's the same problem as BGRA color packing, here is D3DFMT_A8R8G8B8, so you need the IMGUI_USE_BGRA_PACKED_COLOR.
And it can't be support Color Emoji Fonts now.

To create a staging texture to upload texture is necessary, it can change the linear texture to optimal texture (tiling texture).
I found all backends, the D3D9 backend is missing the feature.

  1. I don't knwon why "poor performance". Converting will cost more performance than copy directly.
  2. I'm a little confused why texture pixel format will cause problem. GetTexDataAsRGBA32 means that the texture data is always RGBA format. We didn't need to care if it is RGBA or BGRA. Ok, I think I should using format D3DFMT_A8B8G8R8?

@Xiliusha Xiliusha changed the title Backends: DX9: several improvements Backends: DX9: several improvements and bug fix Feb 26, 2021
@metarutaiga
Copy link

metarutaiga commented Feb 26, 2021

  1. I don't knwon why "poor performance". Converting will cost more performance than copy directly.
  2. I'm a little confused why texture pixel format will cause problem. GetTexDataAsRGBA32 means that the texture data is always RGBA format. We didn't need to care if it is RGBA or BGRA. Ok, I think I should using format D3DFMT_A8B8G8R8?

You found a bug here since imgui support Color Emoji.
the D3DFMT_A8B8G8R8 is not a good format in D3D9 that fewer video card support D3DFMT_A8B8G8R8.

@Xiliusha
Copy link
Author

Xiliusha commented Feb 26, 2021

  1. I don't knwon why "poor performance". Converting will cost more performance than copy directly.
  2. I'm a little confused why texture pixel format will cause problem. GetTexDataAsRGBA32 means that the texture data is always RGBA format. We didn't need to care if it is RGBA or BGRA. Ok, I think I should using format D3DFMT_A8B8G8R8?

You found a bug here since imgui support Color Emoji.
the D3DFMT_A8B8G8R8 is not a good format in D3D9 that fewer video card support D3DFMT_A8B8G8R8.

It seems that if I using IMGUI_USE_BGRA_PACKED_COLOR, GetTexDataAsRGBA32 will generate BGRA format texture data.
This is a good news. I don't need to worry about the support of D3DFMT_A8B8G8R8.
image

test code

#include "imgui_freetype.h"

ImFontGlyphRangesBuilder builder;
builder.AddRanges(io.Fonts->GetGlyphRangesDefault());
builder.AddText(u8"😀");
ImVector<ImWchar> ranges;
builder.BuildRanges(&ranges);
ImFontConfig cfg;
cfg.FontBuilderFlags = ImGuiFreeTypeBuilderFlags_LoadColor;
ImFont* font = io.Fonts->AddFontFromFileTTF("c:\\Windows\\Fonts\\SEGUIEMJ.ttf", 16.0f, &cfg, ranges.Data);

@Demonese
Copy link
Contributor

Demonese commented Feb 28, 2021

5. "code clean"

Remove unused DirectInput header

6. Texture pixel format

if defined IMGUI_USE_BGRA_PACKED_COLOR, using BGRA32 pixel format to create font altas texture.

#ifndef IMGUI_USE_BGRA_PACKED_COLOR
    D3DFORMAT texture_format = D3DFMT_A8B8G8R8; // RGBA32, be aware that this format is not widely supported on Windows platfrom
#else
    D3DFORMAT texture_format = D3DFMT_A8R8G8B8; // BGRA32
#endif

7. New Direct3D9 backed implement: programmable rendering pipeline

Why?
NO vertex convertion (if IMGUI_USE_BGRA_PACKED_COLOR defined)!
(see commits)

if (D3D_OK != g.d3d9Device->CreateVertexDeclaration(layout, &g.d3d9InputLayout))
return false;

// Create vertex shader (vs_3_0)

Choose a reason for hiding this comment

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

You can use vs_1_1 instead of vs_3_0, also ps_2_0 instead of ps_3_0.
There are not using Shader Model 3.0 Features here.

Also you can see my assembly shader code that more readable and editable without shader compiling.
https://github.com/metarutaiga/xxGraphic/blob/master/xxGraphicD3DAsm.cpp

@metarutaiga
Copy link

If the vertex layout is not match in the fixed-function, we can define below in IMGUI_USER_CONFIG.

#define IMGUI_OVERRIDE_DRAWVERT_STRUCT_LAYOUT struct ImDrawVert { ImVec2 pos; float z; ImU32 col; ImVec2 uv; }

But we can't set Z of ImDrawVert without modified imgui, we must use custom allocator to clear it.

static void* MallocWrapper(size_t size, void* user_data)
{
    IM_UNUSED(user_data);
    return calloc(size, 1);
}
static void FreeWrapper(void* ptr, void* user_data)
{
    IM_UNUSED(user_data);
    free(ptr);
}

And set the function before initialing imgui.

ImGui::SetAllocatorFunctions(MallocWrapper, FreeWrapper);

@Demonese
Copy link
Contributor

Demonese commented Feb 28, 2021

If the vertex layout is not match in the fixed-function, we can define below in IMGUI_USER_CONFIG.

#define IMGUI_OVERRIDE_DRAWVERT_STRUCT_LAYOUT struct ImDrawVert { ImVec2 pos; float z; ImU32 col; ImVec2 uv; }

But we can't set Z of ImDrawVert without modified imgui, we must use custom allocator to clear it.

static void* MallocWrapper(size_t size, void* user_data)
{
    IM_UNUSED(user_data);
    return calloc(size, 1);
}
static void FreeWrapper(void* ptr, void* user_data)
{
    IM_UNUSED(user_data);
    free(ptr);
}

And set the function before initialing imgui.

ImGui::SetAllocatorFunctions(MallocWrapper, FreeWrapper);

Thanks for your suggestion.
I found that many online d3d9 tutorials only mention about fixed-pipline or using Effect9 framework (DirectX SDK, but it is legecy/abandoned), so I write a programmable rendering pipeline implement (vs/ps_3_0 or vs_1_1/ps_2_0 is not the most importent).
In Visual Studio 2019, the lowest supported shader model is 2_0 (HLSL compiler), I don't known how to compile vs_1_1 but vs_2_0 and ps_2_0 are compiled successfully.
image
As for z, if we can using programmable rendering pipeline, define a custom input layout is the best choice.

2_0 & 3_0 asm compare

(vs_3_0 -> vs_2_0)
image
(ps_3_0 -> ps_2_0)
image

ocornut added a commit that referenced this pull request Mar 2, 2021
ocornut pushed a commit that referenced this pull request Mar 2, 2021
ocornut added a commit that referenced this pull request Mar 2, 2021
@ocornut
Copy link
Owner

ocornut commented Mar 2, 2021

Hello,

There are a lots of things here so it's going to be a bit difficult to catch on everything.

  1. BGRA color packing (also fix a bug)

OK, will merge.

  1. Bug fix: I think we forget to validate device and texture before drawing. My game crashes in some cases because of this

I think this is not desirable. We don't want to "silently fail". If the texture or device are not set there is a problem with your setup. You can perform the same test before calling ImGui_ImplDX9_RenderDrawData() as well. Not merging.

  1. The ordering of state recovery
    I don't known why, but this did fix a bug in my game. (see commits)

I'd like you to confirm you are 1000% sure this makes a difference. Could you make a video showing with/without the bug and with only this code difference?

  1. Static texture. We can create static texture to improve performance.

I would like this to come with proof/comparison of a net effect in a recent driver + to be implemented without the COM helper. We will however transition to a dynamic atlas this year and therefore I don't think this part will be as meaningful.

  1. "code clean": Remove unused DirectInput header

OK will merge, But your commit has other/unrelated changes there, so I manually applied it over another commit. I also applied the changes in the three main.cpp files (DX9,10,11). This is all squashed along with (1) in commit eb57484.

  1. Texture pixel format: if defined IMGUI_USE_BGRA_PACKED_COLOR, using BGRA32 pixel format to create font altas texture.

This was indeed an issue but instead of merging your solution I have used 23ab497 +
4537d98. This ensure extra compatibility and also avoid conversion if not needed.

  1. New Direct3D9 backed implement: programmable rendering pipeline
    Why? NO vertex convertion (if IMGUI_USE_BGRA_PACKED_COLOR defined)!

That's not worth the extra backend and maintenance effort IHMO, so I don't think I am going to merge this.
If somehow the very minimal amount of changes could be integrated as part of the existing dx9 backend I would consider it.

With the 3 pushed commits this should conflict.
I would suggest you run an interactive rebase to cleanup the PR from already merged features (skip merged 1, remove 2, skip merged 5 without unrelated stuff, skip redone 6) then force-push with less remaining commits then if you want we can discuss the remaining items.

I'm particularly interested in (3).
I think (4) maybe can be ignored due to transition to dynamic atlas anyway.
And (7) if you think is important could be redone inside existing DX9 backend?

Thank you.

@Xiliusha
Copy link
Author

Xiliusha commented Mar 2, 2021

Thanks @ocornut .

  1. I think this is not desirable. We don't want to "silently fail". If the texture or device are not set there is a problem with your setup. You can perform the same test before calling ImGui_ImplDX9_RenderDrawData() as well. Not merging.

I understand. But, ImGui_ImplDX9_RenderDrawData didn't return any value, nor any other API to do validate. Should we export a API to validate device, or return result in ImGui_ImplDX9_RenderDrawData?

  1. I'd like you to confirm you are 1000% sure this makes a difference. Could you make a video showing with/without the bug and with only this code difference?

This is a complex problem... Just like #3857 , maybe a d3d9 bug. In fact, IDirect3DStateBlock is very confusing, it might have some unexpected behavior (for example, I don't known why you backup transforms manually, is it a bug?). I will remove the commit Backends: DX9: restore transform after applying state block and using IntelGPA to find more details.

  1. I would like this to come with proof/comparison of a net effect in a recent driver + to be implemented without the COM helper. We will however transition to a dynamic atlas this year and therefore I don't think this part will be as meaningful.

Ok, dynamic atlas often using staging texture to upload texture data, this still provide some performance improvement. I will move this commit to another branch. It's useful for projects that use older versions of Dear ImGui.

This was indeed an issue but instead of merging your solution I have used 23ab497 +
4537d98. This ensure extra compatibility and also avoid conversion if not needed.

Ok, I will move this commit to another branch. It's useful for projects that use older versions of Dear ImGui. @Demonese It's time for you to get to work.

@Demonese Demonese force-pushed the imgui_impl_d3d9_misc branch from 7d600fd to c44587a Compare March 2, 2021 16:49
@metarutaiga
Copy link

  1. I'd like you to confirm you are 1000% sure this makes a difference. Could you make a video showing with/without the bug and with only this code difference?

This is a complex problem... Just like #3857 , maybe a d3d9 bug. In fact, IDirect3DStateBlock is very confusing, it might have some unexpected behavior (for example, I don't known why you backup transforms manually, is it a bug?). I will remove the commit Backends: DX9: restore transform after applying state block and using IntelGPA to find more details.

I think the state block is a lazy state backup / restore controller.
If you developed a render engine ever, you should know making a state controller is necessary.
It can reduce many duplicated calls.
Then you use your Graphic Wrapped API to make your backend.

// A RenderState backup / restore, I do not test yet, I just wrote in the markdown only
DWORD RenderState[384];
BOOL RenderStateDirty[384];
DWORD RestoreStateArray[384];
BOOL RenderStateBackup;
DWORD RenderStateBackupIndex;
void SetRenderState(DWORD index, DWORD value)
{
    if (RenderStateBackup == false)
    {
        if (RenderState[index] == value)
            return;
        RenderState[index] = value;
    }
    else if (RenderStateDirty[index] == false)
    {
       RenderStateDirty[index] = true;
       RestoreStateArray[RenderStateBackupIndex++] = index;
    }
    device->SetRenderState(index, value);
}
void BackupRenderState()
{
    RenderStateBackup = true;
    RenderStateBackupIndex = 0;
}
void RestoreRenderState()
{
    if (RenderStateBackup)
    {
        RenderStateBackup = false;
        for (int i = 0; i < RenderStateBackupIndex; ++i)
        {
            int index = RestoreStateArray[I];
            device->SetRenderState(index, RenderState[index]);
        }
    }
}

In my opinion, the all backends are just templates, and the all samples are just standalone programs.

@Xiliusha Xiliusha changed the title Backends: DX9: several improvements and bug fix Backends: DX9: programmable rendering pipeline version implement (Dear @Demonese) Mar 2, 2021
@Demonese
Copy link
Contributor

Demonese commented Mar 2, 2021

@ocornut according to your suggestion, I and @Xiliusha modified this branch and pr. Thanks.

And (7) if you think is important could be redone inside existing DX9 backend?

Ok, I will. But currently I will maintain this impl in a independent cpp file.

@ocornut
Copy link
Owner

ocornut commented Mar 2, 2021

(2) I think this is not desirable. We don't want to "silently fail". If the texture or device are not set there is a problem with your setup. You can perform the same test before calling ImGui_ImplDX9_RenderDrawData() as well. Not merging.

I understand. But, ImGui_ImplDX9_RenderDrawData didn't return any value, nor any other API to do validate. Should we export a API to validate device, or return result in ImGui_ImplDX9_RenderDrawData?

It should crash if the device is not setup.
If it doesn't "nicely crash" we can add IM_ASSERT(g_pd3dDevice) and IM_ASSERT(g_FontTexture) with comments.
It is users responsibility to initialize the backend.

(3) This is a complex problem... Just like #3857 , maybe a d3d9 bug.

I would accept this could be a bug and would half-blindly merge if you can show me a video running both versions with only that change in the code and absolutely nothing else. I just want to make sure you didn't make that change along with other changes and were confused by other side effects/bugs.

I'm not sure I understand this comment:
#3844 (comment)

@ocornut according to your suggestion, I and @Xiliusha modified this branch and pr. Thanks.

I am not sure I understand this either. Your PR now still add a new file. My suggestion was that we can only add this if it's added to existing backend with minimal change.

Thanks :)

@Demonese
Copy link
Contributor

Demonese commented Mar 2, 2021

I am not sure I understand this either. Your PR now still add a new file. My suggestion was that we can only add this if it's added to existing backend with minimal change.

Next time I update this branch, they will move to imgui_impl_dx9.cpp.
Currently I add a new cpp file because I don't known how you expect to switch pipeline mode. Should I add a bool parameter to ImGui_ImplDX9_Init?

@ocornut
Copy link
Owner

ocornut commented Mar 2, 2021

I honestly don't know either at this point. This is the first time I even hear of this DX9 feature.
I need to be honest and want to say this would only be desirable to merge if the change was simple and harmless (in term of building/linking compatibility), and I don't know enough about DX9 to tell in advance. If the PR looks super simple and obvious it can be merged.

Since this is a relatively obscure feature (it seems?) I would suggest to add flags to ImGui_ImplDX9_Init() or a separate function call to enable it.

@Demonese Demonese force-pushed the imgui_impl_d3d9_misc branch from c44587a to 00c9d82 Compare March 3, 2021 06:05
@Demonese
Copy link
Contributor

Demonese commented Mar 3, 2021

I honestly don't know either at this point. This is the first time I even hear of this DX9 feature.
I need to be honest and want to say this would only be desirable to merge if the change was simple and harmless (in term of building/linking compatibility), and I don't know enough about DX9 to tell in advance. If the PR looks super simple and obvious it can be merged.

Since this is a relatively obscure feature (it seems?) I would suggest to add flags to ImGui_ImplDX9_Init() or a separate function call to enable it.

well, as you can see, It does have a little performance boost (from about 500us to 400us, during ImGui_ImplDX9_RenderDrawData)

Outdated test results

test code:

#include <stdio.h>
#include <Windows.h>
LARGE_INTEGER freq, t1, t2;
int timer = 0;
double times = 0.0f;

// Render function.
void ImGui_ImplDX9_RenderDrawData(ImDrawData* draw_data)
{
    ::QueryPerformanceCounter(&t1);

    ...

    ::QueryPerformanceCounter(&t2);
    times += (double)(t2.QuadPart - t1.QuadPart) * 1000000.0 / (double)freq.QuadPart;
    timer += 1;
    if ((timer % 60) == 0)
    {
        static char buffer[32];
        snprintf(buffer, 32, "%.3f\n", times / 60.0);
        OutputDebugStringA(buffer);
        times = 0.0f;
    }
}

bool ImGui_ImplDX9_Init(IDirect3DDevice9* device)
{
    QueryPerformanceFrequency(&freq);
    
    ...
}
  • fixed pipeline
    fixed
  • shader pipeline
    shader

In my design, it can automatically enable support for programmable rendering pipelines without more consideration. (API compatible!)
I copied your coding style and it can compiles in C++98 (no build issue!).
It is very compatible and does not require links to any other libraries (only d3d9.lib, no link issue!).

However, the disadvantage is that I modify a lot of places. I realized that if we were to develop dynamic font atlas feature, this pr would take a long time for me to merge others changes.

Thanks!

@metarutaiga
Copy link

When g_IsShaderPipeline is true, we need to backup shader constant, and don't need to backup Fixed-Function transforms.

D3DMATRIX last_world, last_view, last_projection;
g_pd3dDevice->GetTransform(D3DTS_WORLD, &last_world);
g_pd3dDevice->GetTransform(D3DTS_VIEW, &last_view);
g_pd3dDevice->GetTransform(D3DTS_PROJECTION, &last_projection);

// Restore the DX9 transform
g_pd3dDevice->SetTransform(D3DTS_WORLD, &last_world);
g_pd3dDevice->SetTransform(D3DTS_VIEW, &last_view);
g_pd3dDevice->SetTransform(D3DTS_PROJECTION, &last_projection);

When g_IsShaderPipeline is true, we can ignore fixed-function render states. Except for D3DRS_ALPHATESTENABLE and D3DRS_FOGENABLE(Shader Model 2.0).

// disable these features, especially lighting
ctx->SetRenderState(D3DRS_CLIPPING, FALSE);
ctx->SetRenderState(D3DRS_CLIPPLANEENABLE, 0);
ctx->SetRenderState(D3DRS_LIGHTING, FALSE);
ctx->SetRenderState(D3DRS_POINTSPRITEENABLE, FALSE);
ctx->SetRenderState(D3DRS_FOGENABLE, FALSE);
ctx->SetRenderState(D3DRS_ALPHATESTENABLE, FALSE);

It seems that D3DRS_SHADEMODE is lost.

g_pd3dDevice->SetRenderState(D3DRS_SHADEMODE, D3DSHADE_GOURAUD);

Can you use DWORD instead of BYTE in the D3DASM bytecode, like SPIR-V bytecode in imgui_impl_vulkan.cpp?

if (D3D_OK != g_pd3dDevice->CreateVertexShader((DWORD*)g_VertexShaderData, &g_pVertexShader))
return false;
if (D3D_OK != g_pd3dDevice->CreatePixelShader((DWORD*)g_PixelShaderData, &g_pPixelShader))
return false;

static const BYTE g_VertexShaderData[] = {

static uint32_t __glsl_shader_frag_spv[] =

@ocornut
Copy link
Owner

ocornut commented Mar 3, 2021

well, as you can see, It does have a little performance boost

I don't understand what I am looking at. Both are 60 FPS. Try to disable vsync and compare?

@Demonese
Copy link
Contributor

Demonese commented Mar 3, 2021

well, as you can see, It does have a little performance boost

I don't understand what I am looking at. Both are 60 FPS. Try to disable vsync and compare?

see bottom output (us), vsync doesn't affect the execution time

@Demonese
Copy link
Contributor

Demonese commented Mar 3, 2021

It seems that D3DRS_SHADEMODE is lost.

D3DRS_SHADEMODE is using for vertex normal interpolation (fixed pipeline lighting), we didn't need it. (Yes, we've been using it the wrong way.)

Can you use DWORD instead of BYTE in the D3DASM bytecode, like SPIR-V bytecode in imgui_impl_vulkan.cpp?

Sorry... this is the compile result of VS2019. We didn't need to convert to DWORD. Although it's actually 4 bytes wide.

@ocornut
Copy link
Owner

ocornut commented Mar 3, 2021 via email

@Demonese
Copy link
Contributor

Demonese commented Mar 3, 2021

Considering all going on in the drivers i wouldn’t trust a printf metrics omitting the swap time, would prefer to see unthrottled measurements

This only effect the rendering, not present. Rendering and present is different, so I only measured the rendering time.

If you want to see obvious FPS boost, I can modify the main.cpp. Direct3D9, 10, 11, 12 all support a new "flip" swap effect (D3DSWAPEFFECT_FLIPEX in Direct3D9), this swap effect will reduce copy during swapping

I now do a extreme test: a large number of vertex

test code:

  • imgui_impl_dx9.cpp
#include <stdio.h>
#include <Windows.h>
LARGE_INTEGER freq, t1, t2;
int timer = 0;
double times = 0.0f;

// Render function.
void ImGui_ImplDX9_RenderDrawData(ImDrawData* draw_data)
{
    ::QueryPerformanceCounter(&t1);

    ...

    ::QueryPerformanceCounter(&t2);
    times += (double)(t2.QuadPart - t1.QuadPart) * 1000000.0 / (double)freq.QuadPart;
    timer += 1;
    if ((timer % 60) == 0)
    {
        static char buffer[32];
        snprintf(buffer, 32, "%.3f\n", times / 60.0);
        OutputDebugStringA(buffer);
        times = 0.0f;
    }
}

bool ImGui_ImplDX9_Init(IDirect3DDevice9* device)
{
    ::QueryPerformanceFrequency(&freq);
    
    ...
}

main.cpp

...

ImDrawList* renderer = ImGui::GetBackgroundDrawList();
for (float x = 0.0f; x < 2048.0f; x += 8.0f)
{
    for (float y = 0.0f; y < 2048.0f; y += 8.0f)
    {
        renderer->AddRectFilled(ImVec2(x, y), ImVec2(x + 4.0f, y + 4.0f), IM_COL32(255, 255, 255, 128), 2.0f, 64);
    }
}

...

Now it will convert more vertices.
Release mode:

  • fixed
    31
  • shader
    32

It did have a little performace improvement. (if IMGUI_USE_BGRA_PACKED_COLOR defined)

@ocornut
Copy link
Owner

ocornut commented Mar 3, 2021

This only effect the rendering, not present. Rendering and present is different, so I only measured the rendering time.

Submitting the contents != rendering. Present is the best way to guarantee everything has been fully processed.

Unfortunately looking at the state of discussions and this PR I am not entirely sure I want to take on the burden of this extra complexity for a largely dead API.

I still encourage you to make the best and neatest PR/patch and then it can be maintained over the DX9 backend and we will add links to your work, but I suggest that you host it.

@Demonese Demonese force-pushed the imgui_impl_d3d9_misc branch from 00c9d82 to 9e53531 Compare March 4, 2021 05:00
@Xiliusha Xiliusha closed this Mar 4, 2021
Demonese pushed a commit to Demonese/imgui that referenced this pull request Mar 5, 2021
Demonese pushed a commit to Demonese/imgui that referenced this pull request Mar 11, 2021
Demonese pushed a commit to Demonese/imgui that referenced this pull request Mar 13, 2021
Demonese pushed a commit to Demonese/imgui that referenced this pull request Mar 16, 2021
Demonese pushed a commit to Demonese/imgui that referenced this pull request Mar 19, 2021
Demonese pushed a commit to Demonese/imgui that referenced this pull request Apr 29, 2021
Demonese pushed a commit to Demonese/imgui that referenced this pull request May 25, 2021
@Demonese Demonese deleted the imgui_impl_d3d9_misc branch June 5, 2021 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants