appleseedhq / appleseed

A modern open source rendering engine for animation and visual effects
https://appleseedhq.net/
MIT License
2.2k stars 329 forks source link

ScenePicker is no longer created everytime we pick something #2805

Open achalpandeyy opened 4 years ago

achalpandeyy commented 4 years ago

Addresses #2754

Apart from the other regular instantiations of TextureStore, it is only created once when we pick an object the first time in the scene or drag and drop a material the first time in the scene.

I've tested the code with all the test cases mentioned in the issue.

If you could think of a more elegant solution than what I've done in MainWindow::slot_open_cornellbox_builtin_project(), please let me know! :)

achalpandeyy commented 4 years ago

@oktomus Did you see the above conversation with @dictoon regarding this issue?

oktomus commented 4 years ago

@achalpandeyy Yes I did. But the user should not be able to "pick" while its rendering. And if the user picks on an empty render and it doesn't find anything, that's ok.

If you leave the previous has_trace_context, do you ever get any errors ?

achalpandeyy commented 4 years ago

If you leave the previous has_trace_context, do you ever get any errors ?

You would get a Read Access Violation if you put has_trace_context there, because then you try to pick when AssemblyTree isn't created.

m_project.get_trace_context().get_assembly_tree().get_nodes().size() == 0 checks if the AssemblyTree has non-zero nodes i.e. it is created.

achalpandeyy commented 4 years ago

TextureStore is created only one time per Project, just before the rendering is started for the first time. Rest of the system holds references to the TextureStore.

achalpandeyy commented 4 years ago

Thanks @achalpandeyy ! Looks great :)

There is one thing that I'm concerned about. Previously, we used to have multiple texture store (I saw one with 0 parameters and 1 with some paramters). And now, we have the same texture store everywhere and we are not sure which parameters are used. I think that's problematic but I don't know how TextureStore are used.

@dictoon Can you have a look on this ?

@oktomus, this is a vaild point. I'd love to test it.

Do you know of any test scenes which use the "texture_store" param? I couldn't find any.

oktomus commented 4 years ago

Do you know of any test scenes which use the "texture_store" param? I couldn't find any.

Have you tried grep store or something inside the test scenes folder ?

achalpandeyy commented 4 years ago

Do you know of any test scenes which use the "texture_store" param? I couldn't find any.

Have you tried grep store or something inside the test scenes folder ?

I haven't tried running grep to find it.

Again, from my conversation with @dictoon on Discord, I've found that no scene is supposed to set these are params. They are characteristic of the machine and not the scene.

There is an option to force-set these params in appleseed.studio's "Rendering Settings". In the event someone actually does use this, a TextureStore would be properly initialized with these params just before rendering starts for the first time (in MainWindow::start_rendering), then the remaining system uses this TextureStore.

dictoon commented 4 years ago

Thanks for your work @achalpandeyy! I've thoroughly checked this PR, and overall it looks very good. I replaced a few #include by forward declarations where this had become possible.

My main issue now has to do with the new requirement (if I'm not mistaken) that the texture store must be manually initialized before starting a render. See my comment on that. We need to find a solution that doesn't require doing any manual initialization.

dictoon commented 4 years ago

It seems that the unit tests don't pass, possibly for the same reason (texture store not initialized).