search

napframework / nap

NAP Framework source code
https://nap-framework.tech
Mozilla Public License 2.0
404 stars 22 forks source link

Small changes to queue family selection in RenderService #6

Closed AartOdding closed 3 years ago

AartOdding commented 3 years ago

Fixed a bug and added the option to require a queue family that can perform compute commands.

cklosters commented 3 years ago

Hey, thanks for the fix. I believe the bug you're mentioning is this one?

if ((queue_properties[i].queueFlags & required_flags) == required_flags)

The old behaviour (pre-compute) worked because Transfer and Graphics bits are (almost) always grouped together, but it's not right to assume that. So yes, this is right, thanks.

Regarding the other compute related changes, can you make a separate PR for that? Since we don't support compute in the engine we should not add a toggle to enable it. That should be part of a bigger change that actually enables compute as a feature in the engine and demonstrates it in a demo. Hope that makes sense.

Feel free to move the bug fix to a separate PR, I'll merge asap.

cklosters commented 3 years ago

I was in the process of releasing a new official build (0.4.2) and cherry picked your fix. This ensures you remain the author. I also mentioned you in the official release notes.

AartOdding commented 3 years ago

Hey, thanks for the cherry pick! Do you want me to create a MR for the compute queue commit? as you said it might not make too much sense to add it now without any other support for compute.

cklosters commented 3 years ago

Probably better to create a new PR (MR) for the compute queue commit when ready. A thing to consider: when adding compute support, you might not want to use / enable graphics at all for your application. Consider for example a command line app that requires no GPU graphics, only compute for some AI related calculations. The queue selection should take that into account, especially when dealing with a GPU that doesn't support graphics operations (which is theoretically possible).

I'll close this PR for now, feel free to shoot in a new one when there's a bit more to test & review!