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

TestSuite: imgui_app.h for Metal #1

Open
jacobfriedman opened this issue Nov 18, 2022 · 11 comments
Open

TestSuite: imgui_app.h for Metal #1

jacobfriedman opened this issue Nov 18, 2022 · 11 comments
Labels
enhancement New feature or request test suite

Comments

@jacobfriedman
Copy link
Contributor

OF course, "// THIS IS FOR OUR OWN USE AND IS NOT SUPPORTED."
It would be nice to have the struct / types / etc clearly defined in the header rather than in the cpp file.

Please let me know if you'd rather not have me add 'issues' such as this. Alternatively, when I have some free time I can tackle it myself and make a request (lmk)

@jacobfriedman jacobfriedman added the enhancement New feature or request label Nov 18, 2022
@ocornut
Copy link
Owner

ocornut commented Nov 18, 2022

Why would you need those structures? What are you trying to solve?

(additionally: what is unclear about that statement in bold that you pasted? ;)

@jacobfriedman
Copy link
Contributor Author

jacobfriedman commented Nov 18, 2022

Ah, nothing is unclear (re: statement in bold)- simply that I understand it's not a priority.

The cpp didn't care to pick up my #define s , even though I scattered them at every point I could. I believe it was a complex issue with my compiler or pipeline (one that I've run out of time to solve).

Nevertheless, it's usually best practice to put those declarations in the header files.

Goal: I'm trying to imbue imgui_app in my program.

@jacobfriedman
Copy link
Contributor Author

Adding to the "why would you need those structures"- I wanted to access GLFW* window after converting the ImGuiApp type to the ImGuiAppGLFW type, but the struct wasn't found (neither was window).

@ocornut
Copy link
Owner

ocornut commented Nov 18, 2022

Goal: I'm trying to imbue imgui_app in my program.

That's the problem.

The whole point is we don't want to provide imgui_app as a general-purpose service for users. Maintaining public backends and examples has been a problematic source of energy drain, taking us away from development of actual dear imgui, we don't want this meta backend to be used by people.

The cpp didn't care to pick up my #define s , even though I scattered them at every point I could.

If your cpp file is compiled from a project definition the define needs to in a project. It's an issue with you using your build system.

e (one that I've run out of time to solve).

Then you are asking about a XY Problem.

@ocornut
Copy link
Owner

ocornut commented Nov 18, 2022

, it's usually best practice to put those declarations in the header files.

I wholly disagree, what's "best practice" in some internet circles are not best practices in some pro circles.
If we were to follow stack-overflow-hacker-news-clique best practices Dear ImGui absolutely wouldn't exist.

@ocornut
Copy link
Owner

ocornut commented Nov 18, 2022

I'm closing this as invalid use, part of the reason those structs are not in .h is we don't want user app to use that code and it is clearly stated in bold in the header. This is internal use. If you want to use it, copy modify it a you wish that's fine but we can't maintain this as a service.

@ocornut ocornut closed this as completed Nov 18, 2022
@jacobfriedman
Copy link
Contributor Author

Thanks. Sorry for taking your time, and I appreciate the quick response- in future I'll have more concrete examples to contribute to this tester.

@ocornut
Copy link
Owner

ocornut commented Nov 18, 2022

Absolutely happy you ask questions and open issues. Anything that can be improved and sturdied should be. In this instance, what should be sturdied is the understanding that this is not code designed to be maintained for others :)

@jacobfriedman jacobfriedman changed the title Move imgui_app declarations from .cpp into imgui_app.h imgui_app.h Updates Dec 24, 2022
@jacobfriedman
Copy link
Contributor Author

I've changed the title of this thread...

Updating to metal, please let me know if you have an inclination to eventually merge into imgui_app (I may simply use a bare .mm file if APPLE is defined.

https://github.com/jacobfriedman/imgui_test_engine/tree/imgui_app_glfw_metal

@ocornut ocornut reopened this Dec 27, 2022
@ocornut ocornut changed the title imgui_app.h Updates imgui_app.h for Metal Dec 27, 2022
@ocornut
Copy link
Owner

ocornut commented Jan 3, 2023

Feels strange to duplicate the entirety of imgui_app.cpp, couldn't something simpler be designed?

@jacobfriedman
Copy link
Contributor Author

I have a working cleaner version, but it needs its own makefile. Not ready just yet.

@ocornut ocornut changed the title imgui_app.h for Metal TestSuite: imgui_app.h for Metal Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test suite
Projects
None yet
Development

No branches or pull requests

2 participants