search

ingowald / optix7course

Apache License 2.0
452 stars 80 forks source link

Update to also compile on OptiX 7.3 #15

Closed otabuzzman closed 3 years ago

otabuzzman commented 3 years ago

Denoiser API changed with OptiX 7.3. Affected examples 11 and 12 updated.

ingowald commented 3 years ago

Ooooh, nice!

Just one question (since I currently can't check it) : looking at the diffs it seems that the actual code changes have conditionals to work with both 7.3 and older versions (as is the right thing to do!); but the findoptix script seems to "require" 7.3, so owl would then only work with 7.3, right? Could you please remove that "required", and check if it still compiles with pre-7.3 before I merge?

I'd like to merge right away, but can't currently check/fix this myself until next week or so...

otabuzzman commented 3 years ago

Changes to configure_optix.cmake script undone. I have no idea why I changed it in the first place as OptiX is selected via OptiX_INSTALL_DIR variable. It even seems the find_package call inside the script isn't of any use at all, because regardless of which version I pass, OptiX 7.0 to 7.3 all compile without errors. But I'm no cmake expert...

otabuzzman commented 3 years ago

I was wrong regarding find_package not being necessary to look for OptiX: the CMAKE command calls FindOptiX.cmake which finally searches for OptiX in a particular version if given. find_package is therefore required after all, and as we depend on OptiX the REQUIRED parameter is ok too, I think.

But there is a VERSION parameter given to find_package which is not in the CMAKE documentation and could thus be removed.

The version value itself might as well be removed as checking is supposed to happen in FindOptiX.cmake which isn't the case: in a Linux environment, the script uses OptiX as defined by the shell variable OptiX_INSTALL_DIR regardless of its version. On Windows FindOptiX.cmake searches a hardcoded list of possible installation directories which I extended for OptiX 7 based on the existing entries.

I removed the OptiX version when calling find_package in configure_optix.cmake because it leads to the wrong assumption there could be version checking which actually isn't the case.

Finally, I checked compilation on Linux for OptiX 7.0, 7.1, 7.2, and 7.3 by setting OptiX_INSTALL_DIR according to my configuration (/usr/local/optix-7.<minor>.0) and each pass works fine.

ingowald commented 3 years ago

perfect - thank you!