Reworking renderer statistics

[Omniflux]

Recently (actually, a month ago) I was trying to figure out what one of the statistics displayed in luxrender meant. After finally tracking their descriptions down in the wiki at http://www.luxrender.net/wiki/LuxRender_0.8_Interface I thought wouldn't it be nice if there was a tooltip for each of these statistics describing what they were for?

And so I began what I thought would be a simple project of 20 to 30 lines of code in mainwindow.cpp.

Attached is a 2827 line patch.

Unfortunately, implementing tooltips over each statistic means each statistic must be added to the GUI individually; liblux only supplies a single statistics string. Or it lets you create your own template....but that does not work with sppm and even if it did, it would require the GUI to know about all the different statistics for all the different renderers liblux might eventually support.

So It Has Come To This. The attached patch replaces the StatsData class, rips out a bunch of unused statistics code, introduces a general interface for fetching statistics from renderers, and finally..........wait for it..........adds tooltips over the statistics in the GUI.

I have written this with some help and feedback from LordCRC and he has requested that it be posted here for discussion as it is a fairly large change. So here it is.

--
Omni Flux

[jeanphi]

Hi,

That's an interesting direction, good idea. My comments are mostly cosmetic:
- in liblux.cmake, you shouldn't need to opt out the compilation of hybridsamplerstatistics.cpp
- why do you add #include "scene.h" to hybridsamplerrenderer.h?

Have you benchmarked the result? Is there a noticeable speed difference with this new scheme?

Jeanphi

[Omniflux]

- in liblux.cmake, you shouldn't need to opt out the compilation of hybridsamplerstatistics.cpp

Tested and changed.

- why do you add #include "scene.h" to hybridsamplerrenderer.h?

I have changed this to #include "transport.h" instead. I do not understand how this was working before; hybridsamplerrenderer.h references SurfaceIntegratorState.

Have you benchmarked the result? Is there a noticeable speed difference with this new scheme?

I will try to run and post these tonight.

[dougal2]

That looks pretty comprehensive - however I'd suggest you check a few things:

1. You've removed the luxPrintableStatistics() API function; perhaps leave that in and forward the call to the new interface and emit a deprecation warning. Changing API interfaces abruptly is never a good idea.
2. You've removed the luxPrintableStatistics() API function; you need to also update the python bindings that use this interface in the python/ directory
3. The same as (1) applies in the python interface
4. You've removed the luxPrintableStatistics() API function; you need to also update the C++ bindings that use this interface in the cpp_api/ directory
5. The same as (1) applies in the cpp_api interface
6. You also need to update luxblend25 which uses the pylux.Context.PrintableStatistics api function.
7. All of the above also applies to the luxCustomStatistics function and any other Lux tool or interface that uses either function.

You get the idea, I'm sure - (1) is probably the most important point I could make.

[Omniflux]

Thanks for the feedback dougal2

I will look at updating the remaining consumers.

Restoring luxPrintableStatistics() and pointing it to the new interface will be simple.

Restoring luxCustomStatistics() is less simple, and I am not sure the work is worth it here, especially as there are no in tree consumers. How do you feel about dropping just this function?

[dougal2]

Restoring luxCustomStatistics() is less simple, and I am not sure the work is worth it here, especially as there are no in tree consumers. How do you feel about dropping just this function?

you're right, I don't think there are any consumers for this one. However, I might be inclined to leave a stub in for now that logs a deprecation message even if there's no equivalent implementation.

[Lord Crc]

Omniflux has made some alterations, and I've pushed them to a new branch so you can review it more easily.

One idea which we've discussed is to move the "terse formatted" stats to a separate class, so instead of

Code: Select all

luxGetStringAttribute("renderer_statistics_formatted", "_recommended_string_short", ...);

we'd have

Code: Select all

luxGetStringAttribute("renderer_statistics_formatted_terse", "_recommended_string", ...);

or "_short", but you get the idea.

Another improvement we discussed is to add the ability to have a "default" attribute for a queryable object. This would be accessed using the name "" or just a null pointer. Classes can then register one of their attributes to be this default one. Thus the above code could then reduce to

Code: Select all

luxGetStringAttribute("renderer_statistics_formatted_terse", NULL, ...);

or in the case of luxblend25

Code: Select all

context.getAttribute('renderer_statistics_formatted_terse')

I still think we can polish this a bit, but as long as we're happy with the api side of things we could continue polishing after merging.

[cwichura]

Will this do anything to correct the broken total statistics when resuming or using slaves?

[Omniflux]

No, I do not believe this will resolve that issue.

[Lord Crc]

Omniflux reworked the "short" statistics stuff, so that you now can select normal or short by object, instead of attribute. The long and short queryable objects share attribute names.

Any further comments?