I was recently using the black_hole warp and noticed a 'type' option I'd 
never used. I could find no documentation on it other than the option 
being mentioned.

The code looks like it tests for and works only for type 0. My own 
testing confirms other than 0 effectively turns the black_hole warp off.

I found this from 2002: 
http://news.povray.org/povray.beta-test/thread/%3C3c44167a@news.povray.org%3E/ 
which looks like there was intent to remove the option from 3.5.

Have I missed something? Is there some current purpose for other than 
type 0?

I can imagine other black hole types - perhaps a whirlpool or oblong 
effect for example - but given the current state, should we remove the 
option and code for now as Ingo apparently planned?

If 0 is the only valid option for type at present, but others are 
planned, should we warn in the parser when it is set to something other 
than 0? Perhaps also comment the conditional in the code on type to save 
a few cyles until other options exist ?

No matter the answer(s) supposing the documentation should be updated to 
match.

Thanks for your time,
Bill P.

Post a reply to this message

Am 09.07.2016 um 00:46 schrieb William F Pokorny:

> I found this from 2002:
> http://news.povray.org/povray.beta-test/thread/%3C3c44167a@news.povray.org%3E/
> which looks like there was intent to remove the option from 3.5.

For the sake of backward compatibility, removing the option is not an
option (no pun intended).

> If 0 is the only valid option for type at present, but others are
> planned, should we warn in the parser when it is set to something other
> than 0? Perhaps also comment the conditional in the code on type to save
> a few cyles until other options exist ?

A warning might be in order, depending on whether we label the setting
as obsolete (in which case we should warn) or intended for future
extensions (in which case we probably shouldn't warn, except maybe if
the user specifies a value other than the supported value of 0).

As for the conditional, if we go for the "future extension"
interpretation I suggest replacing it with a switch() and making it the
default branch. This way we keep the logic there, but make it easy for
the compiler to optimize it away. (If we do that, we should add a
warning to the parser if a value other than 0 is specified, as the
behaviour will change.)

> No matter the answer(s) supposing the documentation should be updated to
> match.

True.

Post a reply to this message

On 07/09/2016 06:53 AM, clipka wrote:
> Am 09.07.2016 um 00:46 schrieb William F Pokorny:
>
>> I found this from 2002:
>> http://news.povray.org/povray.beta-test/thread/%3C3c44167a@news.povray.org%3E/
>> which looks like there was intent to remove the option from 3.5.
>
> For the sake of backward compatibility, removing the option is not an
> option (no pun intended).
>
:-) Ah yes, think you are right.

>> If 0 is the only valid option for type at present, but others are
>> planned, should we warn in the parser when it is set to something other
>> than 0? Perhaps also comment the conditional in the code on type to save
>> a few cyles until other options exist ?
>
> A warning might be in order, depending on whether we label the setting
> as obsolete (in which case we should warn) or intended for future
> extensions (in which case we probably shouldn't warn, except maybe if
> the user specifies a value other than the supported value of 0).
>
> As for the conditional, if we go for the "future extension"
> interpretation I suggest replacing it with a switch() and making it the
> default branch. This way we keep the logic there, but make it easy for
> the compiler to optimize it away. (If we do that, we should add a
> warning to the parser if a value other than 0 is specified, as the
> behaviour will change.)
>

OK. I'll work toward the "compatibility and future extension" switch() 
change and warn on non-zero setting.

>> No matter the answer(s) supposing the documentation should be updated to
>> match.
>
> True.
>

I'll first create a branch for the change and submit a pull request for 
review. After refinement/adoption, I'll work up the documentation change 
to match.

Thanks,
Bill P.

Post a reply to this message