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

Follow ImGui binding examples a bit more closely #78

Open
eliasdaler opened this issue May 13, 2019 · 4 comments
Open

Follow ImGui binding examples a bit more closely #78

eliasdaler opened this issue May 13, 2019 · 4 comments
Milestone

Comments

@eliasdaler
Copy link
Contributor

So, currently we have this: ImGui::SFML::Render(target) which calls ImGui::Render internally, while in other ImGui binding examples it should be more like this:

ImGui::Render(); // prepare render data
ImGui::SFML::Render(target);

Note that this will be a breaking change, so I'll release v3.0 if we agree on this.
This is needed to save future users from just calling ImGui::Render instead of ImGui::SFML::Render and being confused.

There's another good thing: if ImGui will have nothing to draw, resetGLStates won't be called on RenderTarget, while now it's called every time.

So, thoughts?

@eliasdaler
Copy link
Contributor Author

Also, following other conventions, we should do

ImGui::SFML::Update(...);
ImGui::NewFrame();

instead of calling ImGui::NewFrame inside "Update". Yeah, it'll not be as automatic as now, but it's explicit.

@ocornut
Copy link

ocornut commented May 13, 2019

There's another good thing: if ImGui will have nothing to draw, resetGLStates won't be called on RenderTarget, while now it's called every time.

You can make this change right away, that's orthogonal to changing the api. Just move the resetGLStates in the drawing function.

ImGui::SFML::Update(...);

Calling this NewFrame() would be consistent with other back-ends.

@Rosme
Copy link

Rosme commented May 13, 2019

I see nothing wrong with that. A major version can (and is somewhat supposed) to have breaking changes. Having a binding that follow the most common practice is for the best. I definitely vote for making this breaking change.

@pinam45
Copy link
Contributor

pinam45 commented May 14, 2019

I think it is worth it to make a breaking change to be more consistent with the other back-ends.

@eliasdaler eliasdaler added this to the 3.0 milestone May 20, 2019
@eliasdaler eliasdaler removed the v3.0 label May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants