Cycles : Improve device selection and handling #5678
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was just the same as Cycles' own `available_devices()` method, but with the additional of a strange "MULTI" device that included every single other device, including both the CUDA and Optix variants of the same NVIDIA card. I don't know what that device was intended for, but it doesn't seem to make sense. While "MULTI" does seem to be used as a value for `CyclesRenderer::m_deviceName`, in that case it seems to mean something else - to use the list of devices gathered into `m_multiDevices`. The one potential downside here is that we now call `available_devices()` more, but internally it keeps track of what has been initialised already to avoid repeat work anyway. And most of those calls are going to disappear in upcoming refactoring anyway.
It isn't a thing any more. Also don't completely hide the threads plug - just disable it, so that the layout doesn't jump around distractingly.
murraystevenson
requested changes
Feb 17, 2024
boberfly
reviewed
Feb 17, 2024
| @@ -114,9 +104,4 @@ const std::string &versionString() | |||
| return cyclesVersion; | |||
| } | |||
|
|
|||
| const std::vector<ccl::DeviceInfo> &devices() | |||
| { | |||
| return cyclesDevices; | |||
There was a problem hiding this comment.
Long ago from what I remember the available_devices() call wasn't reentrant and gave different results after the first call, giving me many headaches until I realised that the Blender code only calls it once and stores the results, but maybe they've fixed that now.
There was a problem hiding this comment.
Thanks for the info on the history here Alex. In my own testing available_devices() seems to always return exactly the same results each time, and a glance at the Cycles source suggests that is expected (it uses a lock and flags to avoid multiple initialisations). Unless we see problems in practice, my preference is for the more minimal approach.
murraystevenson
approved these changes
Feb 20, 2024
- Remove all four member variables previously used for device tracking, and just use `m_sessionParams.device`. - Fix bug where we never actually visited the code path for warning about the CPU fallback when trying to mix GPU and OSL. - Move all device matching logic into a single place in `option()` (previously was spread between `option()`, `init()` and `getCyclesDevices()`). - Don't hardcode the device types - use a map for tracking indices per type. - Use `StringAlgo::matchMultiple()` instead of custom splitting and matching logic. - Consult `DeviceInfo:has_osl` rather than assuming only the CPU supports OSL. Not important now, but will be necessary when we eventually compile with Optix/OSL support. This removes around 75% of the device handling code, while keeping the functionality and matching logic the same.
- Add presets widget instead of string field. - Don't hardcode the list of device types. - Don't show types that aren't available. - Organise presets into menus by type. - Only show the "All" options when two or more devices of the same type exist. The "Custom" mode is a get out of jail free card if you need to do something else, but for the vast majority of users the most important thing is clear and obvious selection of their one GPU.
johnhaddon
force-pushed
the
cyclesDevices
branch
from
b54a5c6
to
3b0cfeb
Compare
4 tasks
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This improves the UI for device selection in CyclesOptions, like so :
It also refactors the device handling logic in the renderer backend, fixing a bug and significantly reducing the amount of code involved. Now I'm a bit more familiar with this part of the codebase, I think I'm in a better place to start considering the device reset stuff from #5101.