Because I cannot attach a text file in github.


The file error.txt is from clang (it stopped before going to far)

icerror.txt is from Intel compiler.

gcc_error.txt is from g++ 4.8

All of them when trying the branch hotfix/github_issue_29


-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'gcc_error.txt' (390 KB) Download 'icerror.txt' (179 KB) Download 'error.txt' (81 KB)

Am 29.06.2014 11:52, schrieb Le_Forgeron:

--------
./base/colour.h: In instantiation of ‘T 
pov_base::GenericColour<T>::MaxAbs() const [with T = float]’:
./base/colour.h:1422:27:   required from ‘T 
pov_base::GenericColour<T>::WeightMaxAbs() const [with T = float]’
backend/render/trace.cpp:999:34:   required from here
./base/colour.h:1456:54: error: no matching function for call to 
‘max(float&, double)’
                  result = max(result, fabs(mColour[i]));
--------

That's a quite interesting error we have here: With T=float, mColour is 
defined as "float mColour[3]", so the compiler should pick "float 
fabs(float)", but the max() signature it is trying to find a match for 
indicates that the compiler rather chooses "double fabs(double)".

So either...:

(A) The compiler is buggy, or

(B) The header files are buggy, declaring "fabs(float)" as returning a 
double instead, in obvious violation of the C++03 standard, or

(C) The header files are buggy, failing to declare "fabs(float)" at all, 
again in obvious violation of the C++03 standard.

So we should probably contact the authors of g++ to fix this...


... or should we? Having had a quick glance at the C++03 standard, 
thruth gave me a roundhouse kick right in the face:

(D) We're not including the right header in the first place.


Just a few weeks ago, I had done some work on work coding style; one 
thing I addressed was the order in which header files should be 
included, as well as which names should be used for C standard headers. 
One thing that had been bugging me most was the inconsistenty in using 
the old C standard header file names vs. the new C++ names (e.g. 
<stdlib.h> vs. <cstdlib>), and I went for the C++ names for the coding 
rules; it had been bugging me to the degree that in all the files I have 
been touching ever since, I replaced all the C header names with the C++ 
names.

Well, it should probably have bugged me even more.

 From ISO-IEC 14882-2003 (aka C++03):
-------------------------------------------------
26 Numerics library
[...]
26.5 C Library

Tables 80 and 81 describe headers <cmath> and <cstdlib> [...], respectively.
[...]
The contents of these headers are the same as the Standard C library 
headers <math.h> and <stdlib.h> respectively, with the following additions:
[...]
     float fabs (float);
[...]
-------------------------------------------------

Read this again, and let it sink in: <cmath> is /not/ a C++ canonical 
name for <math.h>. It is an entirely different library, with /added/ 
functionality over <math.h>.

Guess which version of <math.h> / <cmath.h> we've been including in 
POV-Ray all the time...


I think it's time to kick out all the C standard header files for good. Now.

Post a reply to this message

Le 30/06/2014 14:25, clipka a écrit :
> Having had a quick glance at the C++03 standard, thruth gave me a
> roundhouse kick right in the face:

I hope you are not too much injured.

Nicely spotted, btw.

-- 
Just because nobody complains does not mean all parachutes are perfect.

Post a reply to this message

Second round, on f42a95b668458bfd8bf626519669b3cdc36f0d3c

* still std::numeric_limits<> but now in base/types.h (nned <limits>)
* texture.cpp, 1266 : default shared_ptr is to nullptr, not worth setting ?
* same for normal.cpp, 593.
* base/colour.cpp:53:22: error: specializing member
‘pov_base::GenericColour<float>::mkDefaultWavelengths’ requires
‘template<>’ syntax
     const MathColour MathColour::mkDefaultWavelengths =
MathColour(RGBColour(0.70, 0.52, 0.48));

* "return GenericPigmentBlendMapPtr(NULL);" might compile better as
"return GenericPigmentBlendMapPtr()" (calling the constructor of
shared_ptr without argument, making a shared_ptr to nullptr.
(otherwise, IIRC, NULL is taken as the argument for the constructor of T
in shared_ptr<T>)

(I'm more familiar with C++11 std::shared_ptr<T>, so I might be wrong
when applied to C++03 & boost . when I need fresh instanced value,
std::make_shared<T>(params for T's constructor...) is my friend)

Thanks for your update.
-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorgcc.txt' (428 KB) Download 'erroricpc.txt' (175 KB)

Am 30.06.2014 18:06, schrieb Le_Forgeron:

> * "return GenericPigmentBlendMapPtr(NULL);" might compile better as
> "return GenericPigmentBlendMapPtr()" (calling the constructor of
> shared_ptr without argument, making a shared_ptr to nullptr.
> (otherwise, IIRC, NULL is taken as the argument for the constructor of T
> in shared_ptr<T>)
>
> (I'm more familiar with C++11 std::shared_ptr<T>, so I might be wrong
> when applied to C++03 & boost . when I need fresh instanced value,
> std::make_shared<T>(params for T's constructor...) is my friend)

Technically you're right in that the NULL is not required, but you're 
wrong on the details: The shared_ptr<T> constructor does /not/ pass its 
arguments to a constructor of T; in fact, it never even constructs an 
instance of T at all. Instead, it takes a /pointer/ to an 
already-created instance of T as an argument (with NULL being a valid 
parameter as well). To construct a T without make_shared (which is not 
available in TR1), you have to invoke:

     shared_ptr<T>(new T(params for T's constructor...));

As for using "return shared_ptr<T>()" instead of "return 
shared_ptr<T>(NULL)", note that those pieces of code should never be 
called in the first place anyway (they all follow either an 
"assert(false)" or an "Error(...)"), and the only reason they even exist 
is to satisfy compilers ("all control paths must return a value"). I 
prefer to go for code clarity there and leave the "NULL" parameter in.

Post a reply to this message

Le 30/06/2014 19:20, clipka nous fit lire :
> Am 30.06.2014 18:06, schrieb Le_Forgeron:
> 
>> * "return GenericPigmentBlendMapPtr(NULL);" might compile better as
>> "return GenericPigmentBlendMapPtr()" (calling the constructor of
>> shared_ptr without argument, making a shared_ptr to nullptr.
>> (otherwise, IIRC, NULL is taken as the argument for the constructor of T
>> in shared_ptr<T>)
>>
>> (I'm more familiar with C++11 std::shared_ptr<T>, so I might be wrong
>> when applied to C++03 & boost . when I need fresh instanced value,
>> std::make_shared<T>(params for T's constructor...) is my friend)
> 
> Technically you're right in that the NULL is not required, but you're
> wrong on the details: The shared_ptr<T> constructor does /not/ pass its
> arguments to a constructor of T; in fact, it never even constructs an
> instance of T at all. Instead, it takes a /pointer/ to an
> already-created instance of T as an argument (with NULL being a valid
> parameter as well). To construct a T without make_shared (which is not
> available in TR1), you have to invoke:
> 
>     shared_ptr<T>(new T(params for T's constructor...));
> 
> As for using "return shared_ptr<T>()" instead of "return
> shared_ptr<T>(NULL)", note that those pieces of code should never be
> called in the first place anyway (they all follow either an
> "assert(false)" or an "Error(...)"), and the only reason they even exist
> is to satisfy compilers ("all control paths must return a value"). I
> prefer to go for code clarity there and leave the "NULL" parameter in.
> 

But it won't compile on gcc with NULL. :-/
(it's a reported error, not just a warning)

(as NULL is long int 0... and that's not transformable into a
shared_ptr<T>.) (nullptr seems fine for C++0x, but it's not in C++03;
does boost have it ?)

I stand corrected about the constructor syntax.

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

I've seen the new commit (583d4c85e9357f06b02af41b41973a46f315fe3c)

It's better, half size of gcc complains, but does not compile yet.

* <limits> needed in safemath.h (for numeric_limits).. it crawls
* New->Blend_Map = NULL; ... same empty shared_ptr story (with
variations about return)
* base/colour.h , still the max() vs fabs() as double and float.



-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorgcc3.txt' (196 KB)

Am 30.06.2014 22:26, schrieb Le_Forgeron:
> I've seen the new commit (583d4c85e9357f06b02af41b41973a46f315fe3c)
>
> It's better, half size of gcc complains, but does not compile yet.
>
> * <limits> needed in safemath.h (for numeric_limits).. it crawls

Uh... no, it's not missing <limits>; that file has been included there 
for ages; it's missing the std:: namespace descriptor...

> * New->Blend_Map = NULL; ... same empty shared_ptr story (with
> variations about return)

Heh, that surplus piece of code is even in there twice... whoops.

> * base/colour.h , still the max() vs fabs() as double and float.

Now this starts getting me puzzled. Can you double-check whether we 
still have any reference to math.h in our code?

Post a reply to this message

Am 30.06.2014 23:33, schrieb clipka:

>> * base/colour.h , still the max() vs fabs() as double and float.
>
> Now this starts getting me puzzled. Can you double-check whether we
> still have any reference to math.h in our code?

I /think/ I can solve this riddle now:

While the C++03 standard is very ambiguous (at least to me) about 
whether the functions provided by <cmath> should live in the global or 
std:: namespace, the C++11 standard clarifies this, indicating that 
<cmath> /must/ provide the functions in the std:: namespace, and /may/ 
also provide them in the global namespace.

It appears to me that the GCC <cmath> header does the most confusing 
thing, by providing only /some/ functions in the global namespace - 
namely those for the "double" data type.


Ready for the next round.

Post a reply to this message

On 1-7-2014 1:43, clipka wrote:
> Ready for the next round.
>

Keep it going!

While this is all gobbledegook to me, I love to read your exchange of 
thoughts :-) It shows the brain at work... or is this play for you guys?

Thomas

Post a reply to this message

Am 01.07.2014 09:41, schrieb Thomas de Groot:
> On 1-7-2014 1:43, clipka wrote:
>> Ready for the next round.
>>
>
> Keep it going!
>
> While this is all gobbledegook to me, I love to read your exchange of
> thoughts :-) It shows the brain at work... or is this play for you guys?
>
> Thomas

Geeks' variation of Mornington Crescent ;-)

Post a reply to this message

Le 01/07/2014 01:43, clipka nous fit lire :

> 
> Ready for the next round.
> 

Seems the only error that remains (in many place) for gcc is about
shared_ptr with NULL. (it might be enough, if you want to keep NULL, to
have a C-old cast in void*, such as

return GenericNormalBlendMapPtr((void*)NULL);

) (as long as to be ugly with C NULL, a C-cast seems not that more to
the vessel; Of course, a default empty constructor is fine.. but we know
theses lines are not to be executed)

icpc (intel compiler) has additional issues:

backend/texture/texture.cpp(2324): error: no default constructor exists
for class "pov::BlendMap<pov::TexturePtr>"
  TextureBlendMap::TextureBlendMap() : BlendMap(TEXTURE_TYPE) {}

It also happens in normal.cpp; (it might be tied to the previous error
about shared_ptr, well, I hope so)

Clang has the same issue with a long as parameters of shared_ptr than
the two others.



-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorcl4.txt' (42 KB) Download 'errorgcc4.txt' (175 KB) Download 'erroricpc4.txt' (12 KB)

Am 01.07.2014 18:31, schrieb Le_Forgeron:

> Seems the only error that remains (in many place) for gcc is about
> shared_ptr with NULL. (it might be enough, if you want to keep NULL, to
> have a C-old cast in void*, such as
>
> return GenericNormalBlendMapPtr((void*)NULL);
>
> ) (as long as to be ugly with C NULL, a C-cast seems not that more to
> the vessel; Of course, a default empty constructor is fine.. but we know
> theses lines are not to be executed)

I've decided to go for a parameterless shared_ptr constructor with a 
comment.

Let me know what ca65fc523763201fde29dffd89c63788d81bdd16 does.

Post a reply to this message

Le 01/07/2014 21:19, clipka nous fit lire :
> Am 01.07.2014 18:31, schrieb Le_Forgeron:
> 
>> Seems the only error that remains (in many place) for gcc is about
>> shared_ptr with NULL. (it might be enough, if you want to keep NULL, to
>> have a C-old cast in void*, such as
>>
>> return GenericNormalBlendMapPtr((void*)NULL);
>>
>> ) (as long as to be ugly with C NULL, a C-cast seems not that more to
>> the vessel; Of course, a default empty constructor is fine.. but we know
>> theses lines are not to be executed)
> 
> I've decided to go for a parameterless shared_ptr constructor with a
> comment.
> 
> Let me know what ca65fc523763201fde29dffd89c63788d81bdd16 does.
> 
You're close, only express.cpp still has errors, at 5 locations (3133,
3139, 3153, 3157 and 3170), or just 3107 (as it's a template).

backend/parser/express.cpp(3107): error: no instance of constructor
"boost::shared_ptr<T>::shared_ptr [with T=pov::TextureBlendMap]" matches
the argument list
            argument types are: (long)
          return shared_ptr<MAP_T>(NULL);
                 ^
          detected during instantiation of "boost::shared_ptr<MAP_T>
pov::Parser::Parse_Blend_List<MAP_T>(int, pov::ColourBlendMapConstPtr,
int) [with MAP_T=pov::TextureBlendMap]" at line 3170



The 3 compilers agree. Just that one and we can try the link :-)
(Well, I cheated, replacing the NULL with nothing to test... )
it linked... but "make check" ends with a core dump.
Reconfiguring with --enable-debug ...


#0  0x0000000000579557 in pov::IsoSurface::Compute_BBox
(this=0x2b634c0dc8e0) at backend/shape/isosurf.cpp:746
#1  0x000000000051d684 in pov::Parser::Parse_Object_Mods
(this=this@entry=0x2b6344004cc0, Object=0x2b634c0dc8e0) at
backend/parser/parse.cpp:7390
#2  0x0000000000525921 in pov::Parser::Parse_Object
(this=this@entry=0x2b6344004cc0) at backend/parser/parse.cpp:6410
#3  0x000000000052b15d in pov::Parser::Parse_Frame (this=0x2b6344004cc0)
at backend/parser/parse.cpp:6720
#4  0x000000000052b9b4 in pov::Parser::Run (this=0x2b6344004cc0) at
backend/parser/parse.cpp:203
#5  0x0000000000599da7 in pov::Task::TaskThread (this=0x2b6344004cc0,
completion=...) at backend/support/task.cpp:171
#6  0x00002b63370faa4a in ?? () from
/usr/lib/x86_64-linux-gnu/libboost_thread.so.1.54.0
#7  0x00002b6337b2d182 in start_thread (arg=0x2b633cd48700) at
pthread_create.c:312
#8  0x00002b6337e3e30d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111


> 0  0x0000000000579557 in pov::IsoSurface::Compute_BBox (this=0x2b634c0dc8e0) at
backend/shape/isosurf.cpp:746
> #1  0x000000000051d684 in pov::Parser::Parse_Object_Mods
(this=this@entry=0x2b6344004cc0, Object=0x2b634c0dc8e0) at
backend/parser/parse.cpp:7390
> #2  0x0000000000525921 in pov::Parser::Parse_Object (this=this@entry=0x2b6344004cc0)
at backend/parser/parse.cpp:6410
> #3  0x000000000052b15d in pov::Parser::Parse_Frame (this=0x2b6344004cc0) at
backend/parser/parse.cpp:6720
> #4  0x000000000052b9b4 in pov::Parser::Run (this=0x2b6344004cc0) at
backend/parser/parse.cpp:203
> #5  0x0000000000599da7 in pov::Task::TaskThread (this=0x2b6344004cc0,
completion=...) at backend/support/task.cpp:171
> #6  0x00002b63370faa4a in ?? () from
/usr/lib/x86_64-linux-gnu/libboost_thread.so.1.54.0
> #7  0x00002b6337b2d182 in start_thread (arg=0x2b633cd48700) at pthread_create.c:312
> #8  0x00002b6337e3e30d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

container is null.

void IsoSurface::Compute_BBox()
{
    container->ComputeBBox(BBox);
    if(Trans != NULL)
    {
        Recompute_BBox(&BBox, Trans);
    }
}



-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

On 01/07/2014 2:56 PM, clipka wrote:
> Am 01.07.2014 09:41, schrieb Thomas de Groot:
>> On 1-7-2014 1:43, clipka wrote:
>>> Ready for the next round.
>>>
>>
>> Keep it going!
>>
>> While this is all gobbledegook to me, I love to read your exchange of
>> thoughts :-) It shows the brain at work... or is this play for you guys?
>>
>> Thomas
>
> Geeks' variation of Mornington Crescent ;-)
>

Beware! of Geeks bearing sub-routines.

-- 
Regards
     Stephen

I solemnly promise to kick the next angle, I see.

Post a reply to this message

Am 01.07.2014 22:17, schrieb Le_Forgeron:

>> Let me know what ca65fc523763201fde29dffd89c63788d81bdd16 does.
>>
> You're close, only express.cpp still has errors, at 5 locations (3133,
> 3139, 3153, 3157 and 3170), or just 3107 (as it's a template).

We should be there now. (Besides the isosurface bug there was also a 
corresponding parametric bug.)

Post a reply to this message

Am 02.07.2014 04:13, schrieb clipka:
> Am 01.07.2014 22:17, schrieb Le_Forgeron:
>
>>> Let me know what ca65fc523763201fde29dffd89c63788d81bdd16 does.
>>>
>> You're close, only express.cpp still has errors, at 5 locations (3133,
>> 3139, 3153, 3157 and 3170), or just 3107 (as it's a template).
>
> We should be there now. (Besides the isosurface bug there was also a
> corresponding parametric bug.)
>

While you're at it, could you please do me a favour and also try 
building the new "feature/colour_model" branch? I did some nasty 
template stuff there, and need to know if the Linux compilers can digest it.

Post a reply to this message

Le 02/07/2014 04:13, clipka nous fit lire :
> Am 01.07.2014 22:17, schrieb Le_Forgeron:
> 
>>> Let me know what ca65fc523763201fde29dffd89c63788d81bdd16 does.
>>>
>> You're close, only express.cpp still has errors, at 5 locations (3133,
>> 3139, 3153, 3157 and 3170), or just 3107 (as it's a template).
> 
> We should be there now. (Besides the isosurface bug there was also a
> corresponding parametric bug.)
> 
Yes. Compile, link, run.

* make check : ok for the 3 (g++, icpc, clang++)

Yet, it seems gcc (only) is too fast on benchmark. (4 seconds on 12 threads)

Rendering benchmark gives the attached pictures (the icpc seems ok,
within usual time, but gcc is bogus and too fast, far too fast)


-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'gcc.png' (76 KB) Download 'icpc.png' (448 KB)

Preview of image 'gcc.png'

Preview of image 'icpc.png'

Le 02/07/2014 12:04, clipka nous fit lire :

> 
> While you're at it, could you please do me a favour and also try
> building the new "feature/colour_model" branch? I did some nasty
> template stuff there, and need to know if the Linux compilers can digest
> it.
> 
You're welcome, until my soon-to-be-holidays (very soon now!).

g++ does not like it.
clang++ neither.
And icpc is not better.

All of them get a problem with ">>" in template.. it should be "> >" to
avoid ambiguity with >> operator... so it fails. But even correcting
that is not enough.

The error log of gcc and icpc are past the 1.8MBytes (the largest is 7
MBytes)... so I just attach the clang one.

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorcolourcl.txt' (589 KB)

Am 02.07.2014 19:07, schrieb Le_Forgeron:
> Le 02/07/2014 12:04, clipka nous fit lire :
>
>>
>> While you're at it, could you please do me a favour and also try
>> building the new "feature/colour_model" branch? I did some nasty
>> template stuff there, and need to know if the Linux compilers can digest
>> it.
>>
> You're welcome, until my soon-to-be-holidays (very soon now!).
>
> g++ does not like it.
> clang++ neither.
> And icpc is not better.
>
> All of them get a problem with ">>" in template.. it should be "> >" to
> avoid ambiguity with >> operator... so it fails. But even correcting
> that is not enough.
>
> The error log of gcc and icpc are past the 1.8MBytes (the largest is 7
> MBytes)... so I just attach the clang one.

Can you please give me the output with ">>" corrected to "> >"? Thanks a 
lot!

Post a reply to this message

Am 02.07.2014 18:44, schrieb Le_Forgeron:

> Yes. Compile, link, run.
>
> * make check : ok for the 3 (g++, icpc, clang++)
>
> Yet, it seems gcc (only) is too fast on benchmark. (4 seconds on 12 threads)
>
> Rendering benchmark gives the attached pictures (the icpc seems ok,
> within usual time, but gcc is bogus and too fast, far too fast)

Did you do a clean build for the gcc version? Maybe one of the earlier 
builds left some crap behind.

If the bug persists even with a clean build, please open a new issue on 
GitHub, as we're no longer talking about build problems now.

Post a reply to this message

Le 02/07/2014 19:19, clipka nous fit lire :
> Am 02.07.2014 19:07, schrieb Le_Forgeron:
>> Le 02/07/2014 12:04, clipka nous fit lire :
>>
>>>
>>> While you're at it, could you please do me a favour and also try
>>> building the new "feature/colour_model" branch? I did some nasty
>>> template stuff there, and need to know if the Linux compilers can digest
>>> it.
>>>
>> You're welcome, until my soon-to-be-holidays (very soon now!).
>>
>> g++ does not like it.
>> clang++ neither.
>> And icpc is not better.
>>
>> All of them get a problem with ">>" in template.. it should be "> >" to
>> avoid ambiguity with >> operator... so it fails. But even correcting
>> that is not enough.
>>
>> The error log of gcc and icpc are past the 1.8MBytes (the largest is 7
>> MBytes)... so I just attach the clang one.
> 
> Can you please give me the output with ">>" corrected to "> >"? Thanks a
> lot!
> 
Here it is, attached.

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorcolourcl2.txt' (555 KB)

Le 02/07/2014 19:22, clipka nous fit lire :
> Am 02.07.2014 18:44, schrieb Le_Forgeron:
> 
>> Yes. Compile, link, run.
>>
>> * make check : ok for the 3 (g++, icpc, clang++)
>>
>> Yet, it seems gcc (only) is too fast on benchmark. (4 seconds on 12
>> threads)
>>
>> Rendering benchmark gives the attached pictures (the icpc seems ok,
>> within usual time, but gcc is bogus and too fast, far too fast)
> 
> Did you do a clean build for the gcc version? Maybe one of the earlier
> builds left some crap behind.
> 
> If the bug persists even with a clean build, please open a new issue on
> GitHub, as we're no longer talking about build problems now.
> 
I rebuilt it two times. So the issue is on its way. :-)

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Am 02.07.2014 19:23, schrieb Le_Forgeron:

>> Can you please give me the output with ">>" corrected to "> >"? Thanks a
>> lot!
>>
> Here it is, attached.

Thanks. Next incarnation of feature/colour_model is ready for testing.

Post a reply to this message

Am 02.07.2014 19:25, schrieb Le_Forgeron:
> Le 02/07/2014 19:22, clipka nous fit lire :
>> Am 02.07.2014 18:44, schrieb Le_Forgeron:
>>
>>> Yes. Compile, link, run.
>>>
>>> * make check : ok for the 3 (g++, icpc, clang++)
>>>
>>> Yet, it seems gcc (only) is too fast on benchmark. (4 seconds on 12
>>> threads)
>>>
>>> Rendering benchmark gives the attached pictures (the icpc seems ok,
>>> within usual time, but gcc is bogus and too fast, far too fast)
>>
>> Did you do a clean build for the gcc version? Maybe one of the earlier
>> builds left some crap behind.
>>
>> If the bug persists even with a clean build, please open a new issue on
>> GitHub, as we're no longer talking about build problems now.
>>
> I rebuilt it two times. So the issue is on its way. :-)

 From what you wrote earlier, I take it that the biscuits scene renders 
fine.

Can you narrow down which feature causes the problems in the benchmark 
scene? Photons? Media? Radiosity?

It would also be helpful if you could figure out which version was the 
last one that compiled & rendered ok. (If the one after that still 
compiled ok, but rendered bogus, that would also be useful to know.)

Post a reply to this message

Le 02/07/2014 19:37, clipka nous fit lire :
> Am 02.07.2014 19:23, schrieb Le_Forgeron:
> 
>>> Can you please give me the output with ">>" corrected to "> >"? Thanks a
>>> lot!
>>>
>> Here it is, attached.
> 
> Thanks. Next incarnation of feature/colour_model is ready for testing.
> 

It's getting bigger (here the clang version, as gcc is above 5M)

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorcolourcl3.txt' (673 KB)

Am 02.07.2014 19:58, schrieb Le_Forgeron:
> Le 02/07/2014 19:37, clipka nous fit lire :
>> Am 02.07.2014 19:23, schrieb Le_Forgeron:
>>
>>>> Can you please give me the output with ">>" corrected to "> >"? Thanks a
>>>> lot!
>>>>
>>> Here it is, attached.
>>
>> Thanks. Next incarnation of feature/colour_model is ready for testing.
>>
>
> It's getting bigger (here the clang version, as gcc is above 5M)

Future versions will be in experimental/colour_model from now on.

Post a reply to this message

Le 02/07/2014 20:49, clipka nous fit lire :
> Am 02.07.2014 19:58, schrieb Le_Forgeron:
>> Le 02/07/2014 19:37, clipka nous fit lire :
>>> Am 02.07.2014 19:23, schrieb Le_Forgeron:
>>>
>>>>> Can you please give me the output with ">>" corrected to "> >"?
>>>>> Thanks a
>>>>> lot!
>>>>>
>>>> Here it is, attached.
>>>
>>> Thanks. Next incarnation of feature/colour_model is ready for testing.
>>>
>>
>> It's getting bigger (here the clang version, as gcc is above 5M)
> 
> Future versions will be in experimental/colour_model from now on.
> 

Beware, gcc errors once un-bzip2-ed is 1 699 869 bytes long in
errorcolor.txt.

clang errors in the other file.

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorcolor.txt.bz2.zip' (13 KB) Download 'errorcolorcl.txt.bz2.zip' (6 KB)

Le 02/07/2014 19:51, clipka nous fit lire :
> Am 02.07.2014 19:25, schrieb Le_Forgeron:
>> Le 02/07/2014 19:22, clipka nous fit lire :
>>> Am 02.07.2014 18:44, schrieb Le_Forgeron:
>>>
>>>> Yes. Compile, link, run.
>>>>
>>>> * make check : ok for the 3 (g++, icpc, clang++)
>>>>
>>>> Yet, it seems gcc (only) is too fast on benchmark. (4 seconds on 12
>>>> threads)
>>>>
>>>> Rendering benchmark gives the attached pictures (the icpc seems ok,
>>>> within usual time, but gcc is bogus and too fast, far too fast)
>>>
>>> Did you do a clean build for the gcc version? Maybe one of the earlier
>>> builds left some crap behind.
>>>
>>> If the bug persists even with a clean build, please open a new issue on
>>> GitHub, as we're no longer talking about build problems now.
>>>
>> I rebuilt it two times. So the issue is on its way. :-)
> 
> From what you wrote earlier, I take it that the biscuits scene renders
> fine.
> 
> Can you narrow down which feature causes the problems in the benchmark
> scene? Photons? Media? Radiosity?
> 
> It would also be helpful if you could figure out which version was the
> last one that compiled & rendered ok. (If the one after that still
> compiled ok, but rendered bogus, that would also be useful to know.)
> 
It might be a long road.
I just went back to compile 3.7-stable with gcc and the image of
benchmark is even a bit different from the one at head with icpc.

There is a change in the clouds, basically the same place and
shape...but some holes...



-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'icpc.png' (448 KB) Download 'stable.png' (439 KB)

Preview of image 'icpc.png'

Preview of image 'stable.png'

Am 02.07.2014 22:01, schrieb Le_Forgeron:

> It might be a long road.
> I just went back to compile 3.7-stable with gcc and the image of
> benchmark is even a bit different from the one at head with icpc.
>
> There is a change in the clouds, basically the same place and
> shape...but some holes...

That's a known side effect of commit 5a081e92, which fixes a bug in the 
way media used to be sampled (see also FS#318).

Post a reply to this message

Am 02.07.2014 21:43, schrieb Le_Forgeron:
> Le 02/07/2014 20:49, clipka nous fit lire :
>> Am 02.07.2014 19:58, schrieb Le_Forgeron:
>>> Le 02/07/2014 19:37, clipka nous fit lire :
>>>> Am 02.07.2014 19:23, schrieb Le_Forgeron:
>>>>
>>>>>> Can you please give me the output with ">>" corrected to "> >"?
>>>>>> Thanks a
>>>>>> lot!
>>>>>>
>>>>> Here it is, attached.
>>>>
>>>> Thanks. Next incarnation of feature/colour_model is ready for testing.
>>>>
>>>
>>> It's getting bigger (here the clang version, as gcc is above 5M)
>>
>> Future versions will be in experimental/colour_model from now on.
>>
>
> Beware, gcc errors once un-bzip2-ed is 1 699 869 bytes long in
> errorcolor.txt.
>
> clang errors in the other file.

I don't really understand why both gcc and clang insist that mColour is 
undefined, when it's a protected member of the publicly inherited base 
class.

Let's see what the latest change does.

Post a reply to this message

Am 02.07.2014 23:56, schrieb clipka:
> Am 02.07.2014 21:43, schrieb Le_Forgeron:
>> Le 02/07/2014 20:49, clipka nous fit lire :
>>> Am 02.07.2014 19:58, schrieb Le_Forgeron:
>>>> Le 02/07/2014 19:37, clipka nous fit lire :
>>>>> Am 02.07.2014 19:23, schrieb Le_Forgeron:
>>>>>
>>>>>>> Can you please give me the output with ">>" corrected to "> >"?
>>>>>>> Thanks a
>>>>>>> lot!
>>>>>>>
>>>>>> Here it is, attached.
>>>>>
>>>>> Thanks. Next incarnation of feature/colour_model is ready for testing.
>>>>>
>>>>
>>>> It's getting bigger (here the clang version, as gcc is above 5M)
>>>
>>> Future versions will be in experimental/colour_model from now on.
>>>
>>
>> Beware, gcc errors once un-bzip2-ed is 1 699 869 bytes long in
>> errorcolor.txt.
>>
>> clang errors in the other file.
>
> I don't really understand why both gcc and clang insist that mColour is
> undefined, when it's a protected member of the publicly inherited base
> class.
>
> Let's see what the latest change does.

I think I got it; the problem is dependent base class name lookup. 
f9d1c6ba should solve the mColour problem, let's see what else crops up.

(I've found by now that the "nasty" part of the "nasty template stuff", 
which I was afraid might not work at all, isn't really that nasty, and 
is actually an established pattern going by the name of "Curiously 
Recurring Template Pattern", aka CRTP.)

Post a reply to this message

Am 03.07.2014 00:30, schrieb clipka:

> I think I got it; the problem is dependent base class name lookup.
> f9d1c6ba should solve the mColour problem, let's see what else crops up.

Forget f9d1c6ba, commit 81627c72 is now the latest and greatest.

Post a reply to this message

Le 03/07/2014 00:49, clipka nous fit lire :
> Am 03.07.2014 00:30, schrieb clipka:
> 
>> I think I got it; the problem is dependent base class name lookup.
>> f9d1c6ba should solve the mColour problem, let's see what else crops up.
> 
> Forget f9d1c6ba, commit 81627c72 is now the latest and greatest.
> 

It's not yet that.

As tomorrow I'm in holidays... if you have a bit of disk space on your
Windows (about 10 Giga ?), you might get faster return by installing
something like virtualbox and putting inside it a virtual machine to
install a linux iso-cd/dvd (ubuntu ?). I recommend at least 10G of
virtual disk size, but you can allocate more. (interestingly, the disk
image used by virtualbox is about the size of used data, but resizing
the whole disk later is problematic, so sizing big at start is better)

(Well, I wouldn't do it on the opposite path (linux->windows), so feel
no obligation).

CRTP is quite usual, but it might not be the simplest to understand when
you start with template (oh well, variadic templates of C++11 are even
stranger...).

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'error81627c72gcc.txt.bz2.zip' (63 KB)

Le 02/07/2014 23:23, clipka nous fit lire :
> Am 02.07.2014 22:01, schrieb Le_Forgeron:
> 
>> It might be a long road.
>> I just went back to compile 3.7-stable with gcc and the image of
>> benchmark is even a bit different from the one at head with icpc.
>>
>> There is a change in the clouds, basically the same place and
>> shape...but some holes...
> 
> That's a known side effect of commit 5a081e92, which fixes a bug in the
> way media used to be sampled (see also FS#318).
> 
Ok, that's one less point to search. Great & thanks.

I will dive into that exploration after my holidays.

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Am 03.07.2014 18:06, schrieb Le_Forgeron:
> Le 03/07/2014 00:49, clipka nous fit lire :
>> Am 03.07.2014 00:30, schrieb clipka:
>>
>>> I think I got it; the problem is dependent base class name lookup.
>>> f9d1c6ba should solve the mColour problem, let's see what else crops up.
>>
>> Forget f9d1c6ba, commit 81627c72 is now the latest and greatest.
>>
>
> It's not yet that.
>
> As tomorrow I'm in holidays...

Have a nice time!

Post a reply to this message

Am 02.07.2014 18:44, schrieb Le_Forgeron:

> Rendering benchmark gives the attached pictures (the icpc seems ok,
> within usual time, but gcc is bogus and too fast, far too fast)

I finally found out what's going on here.

The implementation of adaptive area lights uses an N*M cache for 
"lightlet" data it has already computed. Previously, yet-uncomputed data 
was flagged by setting the respective colour's red channel to -1. I had 
changed this to use a NaN ("Not-a-Number") value instead, to avoid 
problems when a user deliberately sets a light source's colour to 
negative red.

Now of course this NaN value has to be tested for; according to IEEE 
standard, a NaN value has the property that it is non-equal to anything, 
even itself. So I wrote the test: "if (red != red)...".

Well, it turns out that the g++ compiler optimizes this comparison away 
when in "-ffast_math" mode.

Post a reply to this message

clipka <ano### [at] anonymousorg> wrote:
> Am 02.07.2014 18:44, schrieb Le_Forgeron:
>
> > Rendering benchmark gives the attached pictures (the icpc seems ok,
> > within usual time, but gcc is bogus and too fast, far too fast)
>
> I finally found out what's going on here.
>
> The implementation of adaptive area lights uses an N*M cache for
> "lightlet" data it has already computed. Previously, yet-uncomputed data
> was flagged by setting the respective colour's red channel to -1. I had
> changed this to use a NaN ("Not-a-Number") value instead, to avoid
> problems when a user deliberately sets a light source's colour to
> negative red.
>
> Now of course this NaN value has to be tested for; according to IEEE
> standard, a NaN value has the property that it is non-equal to anything,
> even itself. So I wrote the test: "if (red != red)...".
>
> Well, it turns out that the g++ compiler optimizes this comparison away
> when in "-ffast_math" mode.

Use std::isnan - it works because IEEE 754 and C++ floats are not exactly the
same, and for most practical purposes NaNs will have a valid and identical
storage in memory. Be aware that NaNs and Infinites may raise exceptions
regardless though.

Post a reply to this message

Am 06.07.2014 13:58, schrieb Thorsten Froehlich:
> clipka <ano### [at] anonymousorg> wrote:
>> Am 02.07.2014 18:44, schrieb Le_Forgeron:
>>
>>> Rendering benchmark gives the attached pictures (the icpc seems ok,
>>> within usual time, but gcc is bogus and too fast, far too fast)
>>
>> I finally found out what's going on here.
>>
>> The implementation of adaptive area lights uses an N*M cache for
>> "lightlet" data it has already computed. Previously, yet-uncomputed data
>> was flagged by setting the respective colour's red channel to -1. I had
>> changed this to use a NaN ("Not-a-Number") value instead, to avoid
>> problems when a user deliberately sets a light source's colour to
>> negative red.
>>
>> Now of course this NaN value has to be tested for; according to IEEE
>> standard, a NaN value has the property that it is non-equal to anything,
>> even itself. So I wrote the test: "if (red != red)...".
>>
>> Well, it turns out that the g++ compiler optimizes this comparison away
>> when in "-ffast_math" mode.
>
> Use std::isnan - it works because IEEE 754 and C++ floats are not exactly the
> same, and for most practical purposes NaNs will have a valid and identical
> storage in memory. Be aware that NaNs and Infinites may raise exceptions
> regardless though.

std::isnan() is not a universal solution either, as it's not C++(03) 
standard; for instance, Microsoft Visual C++ doesn't have it (it has an 
_isnan() function instead). And I haven't tested whether it actually 
solves the problem - according to the gcc documentation, -ffast_math 
means that the compiler doesn't know anything about infinities and NaNs 
at all, and the std::isnan function might just as well always return false.

So while it is now clear what's happening, this is not solved yet, so I 
have my work cut out for me tonight.

Post a reply to this message

clipka <ano### [at] anonymousorg> wrote:
> Am 06.07.2014 13:58, schrieb Thorsten Froehlich:
> > clipka <ano### [at] anonymousorg> wrote:
> >> Am 02.07.2014 18:44, schrieb Le_Forgeron:
> >>
> >>> Rendering benchmark gives the attached pictures (the icpc seems ok,
> >>> within usual time, but gcc is bogus and too fast, far too fast)
> >>
> >> I finally found out what's going on here.
> >>
> >> The implementation of adaptive area lights uses an N*M cache for
> >> "lightlet" data it has already computed. Previously, yet-uncomputed data
> >> was flagged by setting the respective colour's red channel to -1. I had
> >> changed this to use a NaN ("Not-a-Number") value instead, to avoid
> >> problems when a user deliberately sets a light source's colour to
> >> negative red.
> >>
> >> Now of course this NaN value has to be tested for; according to IEEE
> >> standard, a NaN value has the property that it is non-equal to anything,
> >> even itself. So I wrote the test: "if (red != red)...".
> >>
> >> Well, it turns out that the g++ compiler optimizes this comparison away
> >> when in "-ffast_math" mode.
> >
> > Use std::isnan - it works because IEEE 754 and C++ floats are not exactly the
> > same, and for most practical purposes NaNs will have a valid and identical
> > storage in memory. Be aware that NaNs and Infinites may raise exceptions
> > regardless though.
>
> std::isnan() is not a universal solution either, as it's not C++(03)
> standard; for instance, Microsoft Visual C++ doesn't have it (it has an
> _isnan() function instead). And I haven't tested whether it actually
> solves the problem - according to the gcc documentation, -ffast_math
> means that the compiler doesn't know anything about infinities and NaNs
> at all, and the std::isnan function might just as well always return false.
>
> So while it is now clear what's happening, this is not solved yet, so I
> have my work cut out for me tonight.

Its in C99 and C++11, but indeed will not work with fast math on for the
respective function. Other than disabling fast math for said function, using a
less likely magic value like HUGE_VALF or a separate store for the flag are all
the portable ways left.

Post a reply to this message

Am 06.07.2014 13:18, schrieb clipka:

> I finally found out what's going on here.
>
> The implementation of adaptive area lights uses an N*M cache for
> "lightlet" data it has already computed. Previously, yet-uncomputed data
> was flagged by setting the respective colour's red channel to -1. I had
> changed this to use a NaN ("Not-a-Number") value instead, to avoid
> problems when a user deliberately sets a light source's colour to
> negative red.
>
> Now of course this NaN value has to be tested for; according to IEEE
> standard, a NaN value has the property that it is non-equal to anything,
> even itself. So I wrote the test: "if (red != red)...".
>
> Well, it turns out that the g++ compiler optimizes this comparison away
> when in "-ffast_math" mode.

Solved as follows:

Configure will now test whether NaNs and/or Infinities are available for 
both double and single precision, survive conversion between the two 
types, and can be tested for.

Attempted ways to test for NaNs include std::isnan(), global isnan(), or 
comparison of the value with itself (or, more precisely, a volatile copy 
thereof, to prevent the test from being optimized away).

Attempted ways to test for Infinities include std::isnan(), global 
isnan(), or comparison of the value's absolute with the highest finite 
value representable by the respective type (or, more precisely, a 
volatile copy thereof, to prevent the test from being optimized away).


If NaNs are available, survive type conversion and are testable, those 
will be used to mark a colour as invalid.

Otherwise, if Infinities are available, survive type conversion and are 
testable, negative infinity will be used instead.

Otherwise, as a last resort, the negative of the highest finite value 
representable by single-precision float will be used instead.


On g++ with -ffast_math, it turns out that negative infinity is a viable 
option; while isinf() is dysfunctional as expected, infinities do 
survive type conversion and can be tested for by comparing the value's 
absolute with the highest finite value representable.

Post a reply to this message