Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

UWP backend and example #6579

Closed
wants to merge 13 commits into from
Closed

UWP backend and example #6579

wants to merge 13 commits into from

Conversation

ahmed605
Copy link

@ahmed605 ahmed605 commented Jul 5, 2023

This PR adds a UWP backend that is based on the Win32 backend, it also adds a UWP C++/WinRT DirectX11 example.

The PR also includes an implementation of clipboard functions for the UWP backend (can be disabled by defining IMGUI_DISABLE_UWP_DEFAULT_CLIPBOARD_FUNCTIONS).

The backend code uses WRL to consume WinRT APIs to stay standard-compliant.

The backend code doesn't use anything Desktop-specific so it should work fine on Xbox, HoloLens, Surface Hub, and even Windows 10 Mobile, it should work down to Windows 10 RTM too.

Screenshot:
image

@sylveon
Copy link

sylveon commented Jul 6, 2023

The example should be C++/WinRT, instead of WRL or C++/CX. I also see missing error checks in the imgui backend.

@ahmed605
Copy link
Author

ahmed605 commented Jul 6, 2023

The example should be C++/WinRT, instead of WRL or C++/CX. I also see missing error checks in the imgui backend.

Done!

@ahmed605
Copy link
Author

@ocornut I'm sorry to bother you, but I was wondering if there are any additional steps or information required in order for this PR to be reviewed. Thanks.

@sakiodre
Copy link
Contributor

I see a few problems here:

  1. You use using namespace too much, this is not necessarily bad, but it doesn't follow imgui coding style either, it might be fine in the App.cpp but you might want to remove it in imgui.cpp.
  2. Why use pch and "app.cpp" instead of "main.cpp" like other examples?
  3. I have no clues how to use UWP, but is it necessary to put everything in the struct App? And can we remove the use of com_ptr?
  4. The description for IMGUI_IMPL_UWP_DISABLE_GAMEPAD need to be put in imconfig.h.

Overall, I think the reason ocornut is hesitant to merge this is because of the inconsistency with imgui code.

@ahmed605
Copy link
Author

ahmed605 commented Aug 12, 2023

I see a few problems here:

  1. You use using namespace too much, this is not necessarily bad, but it doesn't follow imgui coding style either, it might be fine in the App.cpp but you might want to remove it in imgui.cpp.
  2. Why use pch and "app.cpp" instead of "main.cpp" like other examples?
  3. I have no clues how to use UWP, but is it necessary to put everything in the struct App? And can we remove the use of com_ptr?
  4. The description for IMGUI_IMPL_UWP_DISABLE_GAMEPAD need to be put in imconfig.h.

Overall, I think the reason ocornut is hesitant to merge this is because of the inconsistency with imgui code.

For 1 and 2, I will fix these, thanks

For 3, no, it's not necessary but it's a common style in UWP apps, about com_ptr, it makes things simpler as you don't have to manually Release things, it's also a common thing in C++ UWP apps too

For 4, right, I forgot that, will fix it

BTW the reason I overused using namespace is because almost all WinRT namespaces are long so inlining them in the code decreases readability

ahmed605 and others added 3 commits August 14, 2023 23:59
…example, renamed App.cpp to main.cpp in the example, disabled XInput on SDKs that didn't allow LoadLibrary under UWP, and moved the description of `IMGUI_IMPL_UWP_DISABLE_GAMEPAD` and `IMGUI_DISABLE_UWP_DEFAULT_CLIPBOARD_FUNCTIONS` to imconfig.h
@ocornut
Copy link
Owner

ocornut commented Aug 15, 2023

Hello Ahmed,

I think you did a good job with this, thank you.

I may not be inclined to merge this with ease because:

  • (1) it's a rather noisy setup (e.g. misc files including 7 png icons in repository, altering solution in a way which i am not entirely sure works with older VS, large clipboard code in imgui.cpp)
  • (2) it's a very foreign ecosystem and technology to me (I am not familiar with it and I think few active people here are).
  • (3) my very personal opinion is that I consider UWP, ahem, a little bit of a dead-born technology: it is an incredibly verbose and tedious API which has no regards for developers needing to work with multiple technologies or porting application. Every game developer I know dreaded porting their game to XBOX solely because of the UWP layer needed to bootstrap some systems. While this is an intimately personal, maybe biased opinion, and arguably we maintain esoteric backends like GLUT, it does add extra weight on my willingness to close eyes on 1 and 2 and adopt code that I will necessarily have to maintain.

That said,

  • I have no doubt this can be useful to many people.
  • I must state again I think your PR is quite good as is.

From my technical POV my only suggestion would be to move the clipboard functions out of imgui.cpp and back into the imgui_impl_uwp.cpp code. While on e.g. Win32 side are many low-level ways to run on the platform without using standard backends (as MANY people have their own mini-engine and are tempted to not use our backend, but we want them to benefit from clipboard out of the box), in the case of UWP application it would be more practical and reasonable to use your backend, and we can even add comments near default clipboard handlers of imgui.cpp to point to your implementation if someone wants to grab it outside of the full backend.

My overall suggestion, if you like it, would be to refactor this into your own repository, e.g. github/ahmed605/imgui_impl_uwp this way we know that you are willing take on potential issues and maintenance. We would link to it from Wiki pages (e.g. https://github.com/ocornut/imgui/wiki/Bindings) and as many places as make sense. For me this would completely solve the (1) and (2) issues, and it gives you flexibility to design the repo in a way that matches best practices of the ecosystem (though I would personally suggest keeping a simple backends/ and examples/ folder matching dear imgui ones).

(A while ago I have registered the https://github.com/dearimgui and one of my idea was to gather semi-official backends in that location but now I'm not sure if it's a good idea for my own sanity)

@ahmed605
Copy link
Author

Hey Omar,

Thank you for your reply.

For (1), it should work down to VS 2015 (if required workloads are installed), on older VS versions VS will just refuse to load the project but will load other projects fine so it shouldn't affect the solution, also the png files are just 19.1kb in total.

About the clipboard code, yeah that's a good point, I will move it into the backend implementation.

For (2), while that's true you can ping me in the future if something broke or some changes need to be made in the backend.

For (3), actually I find the WinRT ABI, APIs, and the UWP platform in general are quite easy to use, yeah they might be hard to initially learn but once you learn it you'll find it pretty easy to use, like for example if you compare the file picker WinRT API with the classic COM file picker API you'll find that the WinRT one is very much easier to use and with much less code lines too, but that's just my personal opinion too.

My overall suggestion, if you like it, would be to refactor this into your own repository, e.g. github/ahmed605/imgui_impl_uwp this way we know that you are willing take on potential issues and maintenance.

I don't think I have enough time to maintain a separate repo unfortunately, so I prefer if this PR ended up merged in this repo instead (I would still make future PRs to maintain backend if needed), but if this is the only option available I will pick it.

So what do you think? should I still make a separate repo or it would be fine merging this PR here after moving the clipboard code?

@ocornut
Copy link
Owner

ocornut commented Aug 15, 2023

also the png files are just 19.1kb in total.

The gut feeling/perception of cruft in the repository (that include file count) is rather important to the philosophy of this project. If there was a way to build an icon-less application without error I would be greatly in favor of that. There is exactly 0 value to shipping icons for our sample application.

(At first glance I'm also noticing I can take e.g. SplashScreen.scale-200.png and compress it losslesly from 7700 bytes to 3549 bytes using e.g. pngout -9 so the files were not properly compressed. But regardless it'd rather not having 8 random icons in the project.)

I don't think I have enough time to maintain a separate repo unfortunately

Regardless of this being maintained this or not, I'm not sure I see the difference for you between having the code here or in your repo? Whereas if it's here I am personally putting myself in line and under some peer pressure for this to be maintained, something that may be difficult for me to achieve.

I guess you can do the clipboard tweak either way (which means the imconfig stuff are gone or moved too?), best to polish as much as possible here.

@ahmed605
Copy link
Author

If there was a way to build an icon-less application without error I would be greatly in favor of that.

unfortunately there isn't, I mean it's possible to create a powershell script (I think we can embed this in the vcxproj) that copies the icons from the VS templates and put them in the example folder but this isn't a proper method and it'd stop working if the user have PS scripts blocked on their system.

I'm not sure I see the difference for you between having the code here or in your repo?

I mean if there was a change in the imgui API for example you would just change the changed API in the backend so I wouldn't need to open a PR in this case, but if I maintain my own repo I'd have to mirror imgui API changes too.

like I will only open a PR if there's a big API change, my code stopped working, there's a bug in the backend that needs to be fixed, or implementing a new imgui feature/API but for small changes I don't think a PR would be needed for that as it would be easy to apply the changes across the backends.

@ocornut
Copy link
Owner

ocornut commented Aug 15, 2023

unfortunately there isn't, I mean it's possible to create a powershell script [...]

I agree it is not desirable.

I mean if there was a change in the imgui API for example you would just change the changed API in the backend so I wouldn't need to open a PR in this case, but if I maintain my own repo I'd have to mirror imgui API changes too.

I guess I sometimes performs search-and-replace when really obvious but it doesn't happen often as, well, we extremely rarely break backends-facing API in such ways. A 5 years copy of old win32 backend would still work today.
The more elaborate changes, new features, that would need testing and UWP specific code anyway.

The way I would handle it would be to monitor the history for imgui_impl_win32.cpp from time to time and mirror changes when they seems to make sense for UWP, but ultimately it you are using this you'll want to do this naturally or when something specific is requested, but if you don't it's unlikely to break.

@ahmed605
Copy link
Author

I see, I moved the clipboard code to the backend so what's the final decision? should I close this PR and move the changes to a new repo?

@ocornut
Copy link
Owner

ocornut commented Oct 16, 2023

Hello Ahmed,

At the moment I would prefer if you hosted this on a separate repo, I will add all links from relevant places e.g. Readme, Wiki.
If it gets maintained and receive increasing interest over time we can reevaluate the situation in a few years.

Note that the comment "[Legacy VK_* values will also be supported unless IMGUI_DISABLE_OBSOLETE_KEYIO is set]" may be removed from both the backend .h and .cpp files.

@ahmed605
Copy link
Author

Hello Omar,

At the moment I would prefer if you hosted this on a separate repo, I will add all links from relevant places e.g. Readme, Wiki.
If it gets maintained and receive increasing interest over time we can reevaluate the situation in a few years.

Alright, will move it to a separate repo ASAP.

Note that the comment "[Legacy VK_* values will also be supported unless IMGUI_DISABLE_OBSOLETE_KEYIO is set]" may be removed from both the backend .h and .cpp files.

I see 👍

@ahmed605
Copy link
Author

@ocornut here's the repo: https://github.com/ahmed605/imgui-uwp
I also added a DX12 example, I will add a readme later explaining how to use the backend (it's not any different than any other imgui backend but I'll add it just for completion)

ocornut added a commit that referenced this pull request Nov 13, 2023
@ocornut
Copy link
Owner

ocornut commented Nov 13, 2023

Added reference/links in README and Wiki.
I will mention the backend in release note for upcoming version. If there's anything you think I can do let me know!

Closing this for now.

@ocornut ocornut closed this Nov 13, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants