mosra / magnum

Lightweight and modular C++11 graphics middleware for games and data visualization
https://magnum.graphics/
Other
4.75k stars 439 forks source link

GL: Switch TransformFeedback attachBuffers to use ArrayView #588

Closed hugoam closed 1 year ago

hugoam commented 2 years ago

Hello !

We are hitting an issue which is that the TransformFeedback::attachBuffers API is quite inpractical to use. Since it only takes a std::initializer_list, there is no way to attach or detach a number of buffers that is not known at compîle time.

In addition to that, there is no detachBuffers, so you have to use the mentioned attachBuffers to detach your buffers, but again, you can only pass a compile-time sized list to it, so in any situation when you don't know the size at compile time, you're basically unable to detach the buffers correctly, save from writing an ugly compile-time jump table redirecting to the correct sized initializer list.

So I'm suggesting to just take an ArrayView in all those functions, which I don't see any disadvantages to, but if you see one please let me know of what other approach you would have in mind to enhance this API for this use case :)

One advantage of taking ArrayView, on the other hand, would be that, if the user really wants to pass initializer lists to those functions, they still can, but the burden of including initializer_list now lies on the includer and not on the TransformFeedback.h header (although I do think I might have forgotten to remove it from there)

As an additional improvement I think we could use an actual Struct instead of std::tuple for the size-offset variant, but I didn't go the extra mile of doing that. Would also be happy to, though, if you think that's a good idea

I'm leaving it as a draft for now until I hear what you think :)

EDIT: Re-reading the PR I realize I had to modify Buffer::bind as well, which I had forgotten. I don't know how widely it is used in general, to deduce if this would be an okay change or not, so happy to hear your thoughts.

hugoam commented 2 years ago

Sorry, I'm switching this to not draft status to see if this allows CircleCI to run, but I don't think it does :/

mosra commented 2 years ago

Sigh, CircleCI, not again... Maybe it'll work on the next push again, given that your previous PR on this repo worked. Oh, it started doing something now, huh... but just for Mac, not Linux. I smell a CircleCI bug.

Could any of this help? https://support.circleci.com/hc/en-us/articles/360008097173-Troubleshooting-why-pull-requests-are-not-triggering-jobs-on-my-organization-

mosra commented 2 years ago

Re CircleCI, compared to e.g. #580 the builds are somehow running on hugoam/magnum instead of mosra/magnum, so that is probably the root of the problem.

Not sure what exactly could fix that, would it be possible for you to somehow stop building the fork ("unfollow" it on CircleCI)? Some more info is here: https://support.circleci.com/hc/en-us/articles/360008097173-Troubleshooting-why-pull-requests-are-not-triggering-jobs-on-my-organization-

hugoam commented 2 years ago

I think it's starting to look like what you want, there is just a minor thing where I got confused by a comment /** @todoc const std::initializer_list makes Doxygen grumpy */, started removing it because I thought it wasn't relevant anymore, but after seeing the other occurences I'm not sure anymore of what it's supposed to mean and if it still needs to be there

codecov[bot] commented 2 years ago

Codecov Report

Merging #588 (c7a76a4) into master (bd5f667) will decrease coverage by 0.00%. The diff coverage is 58.62%.

@@            Coverage Diff             @@
##           master     #588      +/-   ##
==========================================
- Coverage   82.24%   82.23%   -0.01%     
==========================================
  Files         531      531              
  Lines       37770    37781      +11     
==========================================
+ Hits        31065    31071       +6     
- Misses       6705     6710       +5     
Impacted Files Coverage Δ
src/Magnum/GL/Buffer.cpp 47.40% <50.00%> (+0.03%) :arrow_up:
src/Magnum/GL/TransformFeedback.cpp 47.72% <54.54%> (-1.11%) :arrow_down:
src/Magnum/GL/Buffer.h 100.00% <100.00%> (ø)
src/Magnum/GL/TransformFeedback.h 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

hugoam commented 2 years ago

Not sure what to do about the coverage thing. It doesn't even make much sense to me how project can be down 15%

hugoam commented 1 year ago

No problem for the delay, it's an open source project hehe 😄 I did a pass on your comments, should look pretty good now, and unfollowed the github repos on CircleCI, waiting for the CI builds now

mosra commented 1 year ago

Merged as 62363094ae521d07d76cb0d007cf7ae14ad1f494 and bc1b9e39d7749416fa3238b22975296ac606772a, thank you!