The Persistence of Vision Raytracer (POV-Ray).

This is the legacy Bug Tracking System for the POV-Ray project. Bugs listed here are being migrated to our github issue tracker. Please refer to that for new reports or updates to existing ones on this system.

Attached to Project: POV-Ray
Opened by Jim Holsenback - 2010-09-30
Last edited by Grimbert Jérôme - 2012-09-08

FS#166 - quick_color does not work

the quick_color feature doesn’t work when +qN or Quality=N is set to 5 or below

Closed by  Grimbert Jérôme
Saturday, 08 September 2012, 12:18 GMT
Reason for closing:  Fixed
Additional comments about closing:  

Tested with official code of 3.7RC6: quick_color works with +qN (N⇐5).

Grimbert Jérôme commented on Saturday, 28 May 2011, 18:23 GMT

Identified... it's worst than quick_color.

The creator of Trace (render/trace.cpp) is called twice per thread.
First with the right value for the quality, and it store/update the TraceThreadData, by the creator of TracePixel (called via trace in TraceTask creator).
Then Radiosity code calls it too (creator of RadiosityFunction, in lighting/radiosity.cpp) with a filter via GetRadiosityQualityFlags(), using a hard coded QUALITY_9.
RadiosityQualityFlags remove Q_AREA_LIGHT and if there is no media, remove Q_VOLUME

I believe the creator of Trace must NOT touch the TraceThreadData's member... (object isolation anyone ?) There is already a TODO at that line in Trace::Trace.
But I'm weak. My proposal so far is to create the Data with 0 (instead of QUALITY_9), and to have it updated in Trace's creator only if it is still 0. (so only the first time).

Change #5441, to be inspected and reported if you (code maintainers) agree.
(tested on my branch: it works (but I'm not happy with it))

Edition: I wrote "creator"... it's "constructor". Hopefully you get it.

Christoph Lipka commented on Saturday, 28 May 2011, 22:13 GMT

As a quick fix, I suggest to move the updating of TraceThreadData's qualityFlags into the TraceTask constructor, as the TODO comment already suggests.

For a later "clean" fix, I'd like to note the following design and implementation flaws that play a role here:

- The TraceThreadData object's original purpose is (as far as I can see) to hold thread-specific data; qualityFlags shouldn't be in there in the first place, as it is a thread-independent "constant". The only reason for it being in there is that Compute_Pigment needs to know whether the quick color feature is active or not. It would be cleaner to place it in the SceneData object, which can be accessed via the TraceThreadData object. In case that is not an option (as it may slow down render a bit), the second cleanest solution would be to supply this information at TraceThreadData creation time.

- The RadiosityFunction constructor should be fixed to use a proper quality flags value instead of QUALITY_9, with the same quick color setting as used otherwise.

- There is no need for a RadiosityFunction to be created in the first place unless radiosity is enabled (which it never is in quick-color mode); a RadiosityFunction object is always created though. (The RadiosityFunctor class definition indicates that there might originally have been the intention to use it as a dummy RadiosityFunction object in case radiosity is disabled.)

BTW, the TraceThreadData object seems to be designed primarily as a data container, so strict object isolation isn't intended. The View object does a lot of fiddling with TraceThreadData's members, for instance.

Grimbert Jérôme commented on Sunday, 29 May 2011, 20:25 GMT

Small comments:
1. I was not requesting object isolation per itself, but I was puzzled by a constructor of an object which updates another in a non obvious way (such as adding itself to a list or set... that is understandable. The 3.7RC3 behaviour of the code is strange)
2. I'm used to Aviation/Nuclear state/Military developping/investigation way of thinking: finding the issue is far more important than finding a guilty coder (In fact, I have probably written worse code than anyone else)
3. I have no idea how to make the clean fix (for the time being ?).

Grimbert Jérôme commented on Sunday, 29 May 2011, 20:48 GMT

Should now be fixed with #5443 (the quick way)

Thorsten Fröhlich commented on Tuesday, 23 August 2011, 05:40 GMT

Should now be fixed with #5443 (the quick way)

Did you leave the TODO in the code so we can fix it completely in the future?

Grimbert Jérôme commented on Tuesday, 23 August 2011, 15:57 GMT

I see no TODO to put in code (at least near the patched sections).

Maybe Christoph has a better idea where to put such comments ?

Christoph Lipka commented on Friday, 26 August 2011, 00:25 GMT

TODO comment added back in (at new code location) with change #5482


Available keyboard shortcuts


Task Details

Task Editing