mosra / magnum-integration

Integration libraries for the Magnum C++11 graphics engine
https://magnum.graphics/
Other
97 stars 44 forks source link

ImGuiIntegration: add support for draw list user callbacks #98

Closed pezcode closed 2 years ago

pezcode commented 2 years ago

Continuation of https://github.com/mosra/magnum-integration/pull/84 with tests

(and an unrelated fix for a failing test in ImGui 1.88 :eyes:)

Closes https://github.com/mosra/magnum-integration/pull/84.

codecov[bot] commented 2 years ago

Codecov Report

Merging #98 (f372b00) into master (fa60304) will increase coverage by 0.09%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
+ Coverage   77.73%   77.82%   +0.09%     
==========================================
  Files          21       21              
  Lines         952      956       +4     
==========================================
+ Hits          740      744       +4     
  Misses        212      212              
Impacted Files Coverage Ξ”
src/Magnum/ImGuiIntegration/Context.cpp 98.63% <100.00%> (+0.03%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update fa60304...f372b00. Read the comment docs.

mosra commented 2 years ago

Amazing, I got nothing to complain about. Feels strange :D

Tangential: seeing the workaround for ImGui < 1.80, would it make sense to again bump the ImGui version that's used on the CI? The 1.72 is probably quite ancient nowadays. I think we discussed this before, but don't remember what the conclusion was.

pezcode commented 2 years ago

Thanks for the speedy review 😌

would it make sense to again bump the ImGui version that's used on the CI? The 1.72 is probably quite ancient nowadays. I think we discussed this before, but don't remember what the conclusion was.

A bump would make sense, yeah. If you don't want to spend CI minutes on two versions, the best one would be what the majority of people are likely using. 1.83 is about a year old, but might be too fresh for your taste πŸ˜„ The good part about changes in this part of the repo is that you can change ImGui versions and run tests in a matter of 4 seconds. So keeping backwards compatibility just requires some minor due diligence on new PRs.

mosra commented 2 years ago

Wasn't there some policy that if an ImGui version gets older than 2 years, it becomes "obsolete"? That's what I would aim for, together with regularly cleaning up code needed only for older versions... like we do with Assimp, for example.

Looking through the releases, 1.78 is from August 2020, so 1.77 would be the oldest one, 1.80 is still some months away :) There are a few projects, demos and examples that don't use ImGui that heavily so they have whatever version was current at the time and don't upgrade ever again. I don't want to break these "just because" but I also don't want to have extra maintenance overhead keeping ancient versions working. I'll bump to 1.77 then, and next time there are additions to this library, we can bump & cleanup again.