Page MenuHome

Cycles Device Selection fixes
ClosedPublic

Authored by Brecht Van Lommel (brecht) on Nov 9 2016, 2:08 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles Device Selection fixes.Nov 9 2016, 2:08 PM
Lukas Stockner (lukasstockner97) set the repository for this revision to rB Blender.

Remove some unwanted additions to the commit.

Sergey Sharybin (sergey) requested changes to this revision.

This patch does not work here at all. First of all devices is not defined in set_devices. Guess it sohuld eb something like devices = self.get_devices(), but then i've got error that list object has no attribute use.

I also have warning about unused compute_device_type_items.

it's annoying to have RNA settings for versioning, might be a trick here (need to think a bit). Maybe @Bastien Montagne (mont29) can give some magic.

For testing multiple devices you can simply push card twice in device_cuda_info().

P.S. %x format warning is still unfixed.

This revision now requires changes to proceed.Nov 9 2016, 2:32 PM

The devices should be self.devices, not sure how that got messed up.
Regarding the unused warning, I get that as well, but I'm not exactly sure why as it's used in line 4208.
Regarding %x: I don't get this warning, neither on GCC 4.8, 5.3 or 6. But, of course these should be unsigned ints nevertheless. To avoid (unsigned int) all over the printf function, I guess it'd be best to have a string_from_pci function in util_string.

Oh, and it turns out that there is another problem with the RNA: The Device Type works fine, but the device itself doesn't because the enum items used to be specified through a callback - now there's only one hardcoded value, so that always is returned. So, it would be great if there was a way to get the string value of the enum for versioning without reintroducing the whole Cycles C API part that dealt with setting the enum items.

I fixed the auto tile size addon in T50001.

Personally I think it's fine to only preserve the compute_device_type and just enable all devices. Yes, it's not strictly backwards compatible if you have multiple GPUs, but I believe that's acceptable between 2.78 and 2.79, or 2.78 and 2.8. It's not worth it reintroducing that whole C API in my opinion.

source/blender/makesrna/intern/rna_userdef.c
54–62

This can be moved into the function that uses this. The reason for the unused variable warning is that this file is compiled twice, once with RNA_RUNTIME disabled.

4205–4216

I don't think there's a good way to avoid exposing these, unless we move the versioning code into C. I would give them some prefix like legacy_ and explain in the description to make it more clear these should not be used anymore.

To recap the situation:

  1. Py API compatibility is broken.

Here @Brecht Van Lommel (brecht) choosed to update scripts to a new API instead, so does this mean we officially accept such a breakage?

  1. Cycles Standalone fails to render on CPU after porting the original commit to the standalone repo.

Says something about "device cpu is unknown" or something like this. This was not yet looked into yet to my knowledge.

  1. It is now almost 2 weeks after original change was committed, meaning changes in defaults etc are becoming more painful.

Either do a compat preserving changes ASAP or get responsible for the change and document it as something known (including communication with the upcoming reports in the tracker).

  1. Interface decision is still fully weird, and it's really not that hard to change it back.

I do not see how the change which moved settings from system panel to an addon settings could ever be considered a good move, especially if this required adding a settings shortcut in the render panel. Nor i want such a shortcuts buttons being added all over the interface now.

Please get interface team involved here!

  1. Py API compatibility is broken.

Here @Brecht Van Lommel (brecht) choosed to update scripts to a new API instead, so does this mean we officially accept such a breakage?

I don't think it is possible to have this feature without breaking API compatibility. A fixed list of choices vs. enabling an arbitrary subset of devices is just different and we can't expose that using the same API. In my opinion the API breakage is in line with what we do when adding or changing features in 2.7x releases. If you don't think it's acceptable, I can revert the change and commit this only to blender2.8, because that's the only solution I see if we want to avoid breaking the API.

  1. Cycles Standalone fails to render on CPU after porting the original commit to the standalone repo.

Says something about "device cpu is unknown" or something like this. This was not yet looked into yet to my knowledge.

  1. It is now almost 2 weeks after original change was committed, meaning changes in defaults etc are becoming more painful.

Either do a compat preserving changes ASAP or get responsible for the change and document it as something known (including communication with the upcoming reports in the tracker).

I was assuming @Lukas Stockner (lukasstockner97) would finish this since it is his patch, but I can take over if he wants.

  1. Interface decision is still fully weird, and it's really not that hard to change it back.

I do not see how the change which moved settings from system panel to an addon settings could ever be considered a good move, especially if this required adding a settings shortcut in the render panel. Nor i want such a shortcuts buttons being added all over the interface now.

I think it's better to have a shortcut in this specific case personally, to solve the problem of users not knowing to configure the devices in the user preferences first, and I see no alternative solution to that. Additionally we shouldn't have to scroll in the System user preferences, it was designed to be fixed size and this feature adds an arbitrary number of devices, but that convention was already broken so we're not making it worse.

Maybe someone on the user interface team has a different solution, I've added the UI team as reviewers to get feedback. If we think following UI conventions regarding shortcuts is more important then I'll move it back to the System tab.

Here's what I'd like to commit:

  • Backwards compatibility only for compute_device_type, the device ID is too complicated to map and only affects multi GPU users.
  • Add userpref.blend version and check that, it's not the same as regular .blend.
  • Rename old property to legacy_compute_device_type, and hide in the UI.
  • Only apply backwards compatibility for users that haven't already resaved their user prefs (<= 2.78.1).
  • Always show device enum in render properties, greyed out if not usable so that users can look at the tooltip to guide them to the user prefs.

Cycles standalone was already fixed in rB69288737caf4: Fix Cycles standalone not finding CPU device after recent changes. as far as I know.

I did intend to finish the patch, but was waiting for answer regarding the device enum situation (and was pretty busy with non-Blender stuff).
Thanks for updating the diff!
Regarding upcoming reports, I'm fine with handling them since my change caused all the issues in the first place...

This revision was automatically updated to reflect the committed changes.