ScenePicker is no longer created everytime we pick something

[achalpandeyy]

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! :)

[oktomus]

Thanks @achalpandeyy ! Caching the scene picker is indeed a good solution. Although, the way your reset it and recreate it is not resilient and does not work with any project.

[oktomus]

Thanks @achalpandeyy ! Amazing work :)

You solution seems to work, but I think it's a bit complicated. The rendertab has already a lot of connections with other controllers, and adding a new connections with the assmebly tree makes the render tab harder to maintain.

I have few questions :

• Have you tried to update the project context each time we pick ? Or simply update it when it's not available ?
• When is the assembly_tree_build signal is emitted compared to viewports creation ?

And finally, the assembly tree being built should never trigger the activation of material and scene picker. That's not its responsibility. But I'm sure you will find a workaround.

EDIT :
You can check MainWindow::update_workspace, a place where you can update the tabs accordingly.

[oktomus]

I don't understand the changes, why the changes you made to has_trace_context make things work ?

[achalpandeyy]

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

[oktomus]

@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]

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]

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.

[oktomus]

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 ?

[achalpandeyy]

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]

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]

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]

Having to manually initialize the texture store before a render is a big problem. In fact, you haven't updated all the other places where we start a render, meaning that rendering there is now broken (appleseed.cli, appleseed.bench, Max plugin, Maya plugin, Blender plugin, Gaffer integration and possibly many others).

[dictoon]

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]

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