POV-Ray

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.

Tasklist

FS#166 - quick_color does not work

Attached to Project: POV-Ray
Opened by Jim Holsenback (jholsenback) - Thursday, 30 September 2010, 10:58 GMT
Last edited by Grimbert Jérôme (Le_Forgeron) - Saturday, 08 September 2012, 12:18 GMT
Task Type Definite Bug
Category Backend → Texture/Material/Finish
Status Closed
Assigned To Grimbert Jérôme (Le_Forgeron)
Operating System All
Severity Low
Priority Normal
Reported Version 3.70 beta 38
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

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

This task depends upon

Closed by  Grimbert Jérôme (Le_Forgeron)
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).
Comment by Grimbert Jérôme (Le_Forgeron) - 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.

Comment by Christoph Lipka (clipka) - 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.

Comment by Grimbert Jérôme (Le_Forgeron) - 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 ?).

Comment by Grimbert Jérôme (Le_Forgeron) - Sunday, 29 May 2011, 20:48 GMT

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

Comment by Thorsten Fröhlich (thorsten) - 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?

Comment by Grimbert Jérôme (Le_Forgeron) - 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 ?

Comment by Christoph Lipka (clipka) - Friday, 26 August 2011, 00:25 GMT

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

Loading...