float parameters are not always floats

[-Vit-]

I feel I've read about this or even mentioned it myself, but I'm not sure (hard to search for)...
Integers passed into float parameters remain integers and are not converted into floats. So dividing any float parameter by an integer can cause bugs if the caller might pass integers.

This script shows a float parameter that's really an int (affects all avisynth versions I tried).

FloatTest(9)

function FloatTest(float x)
{
BlankClip()
Subtitle( \
"x=" +string(x )+"\n"+ \
"x is int:" +string(IsInt(x) )+"\n"+ \
"x is float:"+string(IsFloat(x))+"\n"+ \
"x/2=" +string(x/2 )+"\n"+ \
"float(x)/2="+string(float(x)/2)+"\n", lsp=12 \
)
}
#Output
#x=9
#x is int:true
#x is float:true
#x/2=4
#float(x)/2=4.500000

The only documentation I found was the little comment on IsFloat that indicates that ints are considered to be floats, which is kinda the problem. That's why x is flagged as both float and int.

This will cause different behavior depending on how functions are called and will cause bugs in realistic scenarios. I was caught out today dividing a float parameter by Width(). Another seemingly innocent example, an expression like this (if radius is an integer > 1):
(sharpness/radius)*factor
will give a different result for these two calls:
DoFunction(sharpness=1.0)
DoFunction(sharpness=1)

Particularly concerning is this can be caused at the user end, and possibly happens in some scripts even now. The user should pass a float but it's not really a user error, it's an error in the language type-handling IMO. There should either be a syntax error or an implicit conversion to honor what the script specifies. The only robust workaround is to cast all float parameters to floats (!!!). Surely that could be done in the language implementation...

Edit: Being careful with use of integers in operators is not entirely sufficient, because you can get the nasty scenario of float parameter divided by float parameter, both passed as integers... :(

[Gavino]

Yes, I fell over this myself a long time ago, but never got round to reporting it. I'm not sure if it's a 'bug' or a 'feature'.

It only leads to a problem on division, so I now make sure when writing functions that any 'float' parameter used in a division is explictly converted to float.

[-Vit-]

It's not only float parameters in divisions that are potentially unsafe, values calculated from the parameters may be too. Whenever you do a floating point division you must ensure that a float literal is involved in some way, either in the division itself, or in the calculation of the variables involved. If not an explicit conversion to float is required. Only a float literal guarantees "floatness", the float parameter type does not.

In a general-purpose language this would be a major problem, but floating point divisions are reasonably rare in Avisynth and literals common. Nevertheless I would say this was a bug in the language rather than a feature. The float 'type' does not tell you a value is a float, and you have to cast a float to a real float - that's gotta be wrong. Automatically converting ints to float parameters should be safe enough surely? You might occasionally have to convert some float values back to integers to call functions (similar to point in script below). And I guess it would break scripts that expected accuracy on *very* large ints passed into float parameters - not a common (or sensible) situation. Anything else?

The problem does have some other minor effects. You can't normally pass a float value into an integer parameter, but you can if the float is actually an int in disguise:

FloatTest(9.0) # Syntax error
FloatTest(9) # Works OK
BlankClip()

function FloatTest(float f) {
IntTest(f)
}

function IntTest(int i) {
return i
}

The script will only execute if you pass the 'wrong' types to each function! Similar effect with % which expects ints

[Gavino]

It's not only float parameters in divisions that are potentially unsafe, values calculated from the parameters may be too. Whenever you do a floating point division you must ensure that a float literal is involved in some way, either in the division itself, or in the calculation of the variables involved. If not an explicit conversion to float is required.
Yes, that's what I do.
I've got so used to it that it's become just another aspect of the int v float distinction that you already have to be careful about when performing any division.

However, I agree an automatic conversion would make it easier to write robust functions, with little practical loss of functionality.
It would be interesting to know if the original designer (Ben R-G) intended this behaviour or if it was an oversight.

[SEt]

Looks like a bug to me. How about this? http://www.mediafire.com/file/s42krfgsbbawlk0/avisynth.7z

[-Vit-]

That seems just fine, float parameters can now be assumed to be floats, thanks SET. [And an MT version too :)]
Although I can't actually take advantage until everyone switches to a version that supports it... :(

Although one minor point remains, is this still sensible:
IsFloat(9.0) = true
IsFloat(9) = true
IsInt(9.0) = false
IsInt(9) = true

[Gavino]

The behaviour of IsFloat() is consistent with the API function of the same name.

[SEt]

If there are no issues I'll post it as next 2.6 MT. You don't really need everyone: who wants new version of your scripts – will need to use adequately new Avisynth version as well. :) I see no reason to use any older Avisynth, and if there is – issues should be addressed, not ignored.

How about allowing int parameters to take float arguments? With proper type conversion and C rounding of course.

[-Vit-]

The behaviour of IsFloat() is consistent with the API function of the same name.
Looking at the source I have to hand (2.58) it doesn't look especially sensible/vital in the API either. Many uses have an int code path already. I'm not suggesting it's an important change, but I don't see why it is the way it is...
How about allowing int parameters to take float arguments? With proper type conversion and C rounding of course.
My immediate reaction is yes, certainly... but do the current syntax errors guard us from any naive scripting mistakes? I can't think of any, but...

[gyth]

crop(3.2, 8.457, -6.213, -.6) ?

[-Vit-]

Yeah, I suppose allowing float pixel values might cause confusion in itself (and crop(3.99,3.99,-3.99,-3.99) would need a "this is how C rounding works" explanation).
Avisynth needs an expert mode...

[Gavino]

If there are no issues I'll post it as next 2.6 MT.
Can you post the source code for your change, please?
I'd like to consider the full ramifications.
I assume it also affects calls to built-in filters and plugins (anything called via env->Invoke()).

How about allowing int parameters to take float arguments? With proper type conversion and C rounding of course.
That's a disgusting idea. :eek:

[SEt]

That's a disgusting idea. :eek:
What is so bad about it compared to int->float? Both passing int32 to float and float to int lose precision. They look the same to me.

Index: avisynth.cpp
===================================================================
RCS file: /cvsroot/avisynth2/avisynth/src/core/avisynth.cpp,v
retrieving revision 1.60
diff -u -r1.60 avisynth.cpp
--- avisynth.cpp 28 Mar 2012 08:42:20 -0000 1.60
+++ avisynth.cpp 18 May 2012 21:36:51 -0000
@@ -1833,7 +1975,11 @@
p += 2;
} else {
if (src_index < args2_count)
- args3[dst_index] = args2[src_index];
+ // SEt: hack to pass integer to float as actually float
+ if (*p == 'f')
+ args3[dst_index] = AVSValue(args2[src_index].AsFloat());
+ else
+ args3[dst_index] = args2[src_index];
src_index++;
dst_index++;
p++;
@@ -1864,7 +2010,11 @@
} else if (args[i].Defined() && !FunctionTable::SingleTypeMatch(q[1], args[i], false)) {
ThrowError("Script error: the named argument \"%s\" to %s had the wrong type", arg_names[i], name);
} else {
- args3[named_arg_index] = args[i];
+ // SEt: hack to pass integer to float as actually float
+ if (q[1] == 'f')
+ args3[named_arg_index] = AVSValue(args[i].AsFloat());
+ else
+ args3[named_arg_index] = args[i];
goto success;
}
} else {
@@ -1879,7 +2029,7 @@

There is also array case where I'm not sure what to do.

[-Vit-]

float->int conversion: I've changed my mind. Thinking as a programmer there's nothing wrong with it - same as many other languages. But many (most) Avisynth scripters are not programmers and would not appreciate the reason/use for that change. It's the precision issue: conversions either way can lose precision but the typical Avisynth int->float conversion is well within the range where no precision is lost. So it's fairly safe to do an implicit conversion there because the non-programmer won't notice. However, almost all float->int conversions will lose notable precision and will confuse non-programmers (crop examples). It saves programmers only a little time. Also, Avisynth doesn't issue warnings so implicit conversions need a little more care.

The float parameter thing needs to be fixed because it's a clear deficiency in the language that can cause problems for experienced or novice alike. float->int is handled correctly already so there's no compelling reason to change it. I don't think we need symmetrical behavior because Avisynth is a special-purpose language.

[SEt]

I'm not strongly proposing float->int conversion, just the symmetrical behavior looks better to me and I've already used to it. As for guarding inexperienced Avisynth users – well, there are so many places they can do wrong that one more, one less... :)

Making int not IsFloat on the other hand is way more dangerous as it disables previously allowed so it's hard to guess what can break. From the purity standpoint it's a logical change though and if implemented should be done on all levels.

[gyth]

1 + 1.1 should be the float 2.1, not the int 2, right?

Python struggled with a similar problem.
"Classic" vs. "True" vs. "Floor" division.
http://docs.python.org/release/2.2.3/whatsnew/node7.html

int->float->int changing the number of frames/audio samples is at least potentially problematic.

[-Vit-]

Passing number of samples/frames into a float would break with the int->float change, but it's probably rare, not sensible, and is already broken in spirit. Integer range in 32-bit float is +/-16777216, anyone passing values of that order into float is lucky it works. Such code should be fixed more than Avisynth remain broken.

Python had a problem with division given its dynamically typed variables. I suppose this thread is a specific example of that, with float parameters being dynamic despite the syntax suggesting otherwise. You can create examples of the division problem without parameters, but they're a bit artificial; kinda scripting mistakes really:

strength = 2 # 0 to 4
percent = Select(strength, 0.0, 25.0, 50, 75.0, 100.0)
factor = percent / 100 # Strength=2 will use int division, others float (because percent is dynamically typed)


# A text-file driven blur. User provides percentages - works if they're floats, but has no effect if they're ints
colorbars(512,512).trim(0,50)
w = Width()
h = Height()
ScriptClip("BicubicResize(int(w * (1-blur/200)), int(h * (1-blur/200))).BicubicResize(w,h)")
ConditionalReader("blur.txt", "blur", false)


1+1.1 is already float 2.1, no change expected there (unless float->int implicit conversion was supported and you pass the result into an int parameter).

[StainlessS]

Just curious, what would you do with an optional float function argument that is not
supplied, would it be an undefined arg of type float?

[Gavino]

Just curious, what would you do with an optional float function argument that is not
supplied, would it be an undefined arg of type float?
No - an undefined argument has its own type, the 'void' type.
http://avisynth.org/mediawiki/Script_variables

[-Vit-]

Arguments not supplied are given the void or 'undefined' type, this is a type that is neither clip, int, float, bool or string.
Edit: Yeah, what Gavino said, lol...

[StainlessS]

No, what I meant was "What would you do", ie if you changed the way it currently works.

[Gavino]

No, what I meant was "What would you do", ie if you changed the way it currently works.
Nothing - that aspect should not change.

[StainlessS]

Thankyou sir, gonna read this thread a couple more times. :)

[IanB]

@StainlessS,

Yes with SEt's patch an optional unspecified float arg would become a float of whatever happened to be in memory as the value, oops! :o

On quick reflection the test if (*p == 'f')needs to become if ( *p == 'f' && args2[src_index].IsInt() )and of course the other test as well.


@All,

This part of the language is indeed messy. Int to Float is not transparent for values > ~2^24.

This is really only a problem for script functions, in plugins the author will probably be using .AsFloat() and will not see the issue unless they explicitly code for it. SEt's proposed patch changes this behaviour, i.e. it will no longer be possible to check if an input arg specified to be a float was actually an Int in drag or a real Float. But does this matter?

Possibly attacking ScriptFunction::Execute might be a better target.

[Gavino]

Yes with SEt's patch an optional unspecified float arg would become a float of whatever happened to be in memory as the value, oops! :o
You're right about the problem and the solution, but it only affects an explicitly supplied 'undefined' value, eg from a wrapper function, such as (silly example):
function f(float "x") {
x = Default(x, 999.9)
BlankClip().Subtitle(string(x))
}

function f2(float "x") { f(x) }

f2()

This is really only a problem for script functions, in plugins the author will probably be using .AsFloat() and will not see the issue unless they explicitly code for it. SEt's proposed patch changes this behaviour, i.e. it will no longer be possible to check if an input arg specified to be a float was actually an Int in drag or a real Float. But does this matter?
I don't think so. If you want to make that sort of distinction, you can use '.' as the type (or 'val' in script functions) and do your own type checking inside the function.

Possibly attacking ScriptFunction::Execute might be a better target.
That would be cleaner in a way, restricting the change just to script functions, although it does introduce an inconsistency.
It would also involve extending the ScriptFunction class to store the parameter types string, but that's simple enough to do.

[SEt]

Ew, I forgot that you can be nasty and pass value 'undefined'. I prefer
if (*p == 'f' && args2[src_index].Defined())
to the IanB's solution as it shows the actual problem.

What about array case? I feel it probably should has conversion too, but I don't know any examples where it's used.

I vote for consistency. And even making int not IsFloat if there are no serious issues with it.

[Gavino]

What about array case? I feel it probably should has conversion too, but I don't know any examples where it's used.
This case is less important as it does not affect user script functions, and any relevant built-in or plugin functions almost certainly use AsFloat() to obtain the values anyway (eg as in Spline()).
But to be wholly consistent, I think the code marked here
} else if (p[1] == '*' || p[1] == '+') {
int start = src_index;
while (src_index < args2_count && FunctionTable::SingleTypeMatch(*p, args2[src_index], strict))
src_index++;
args3[dst_index++] = AVSValue(&args2[start], src_index - start); // can't delete args2 early because of this
would need to be changed to
while (src_index < args2_count && FunctionTable::SingleTypeMatch(*p, args2[src_index], strict)) {
if (*p == 'f') args2[src_index] = AVSValue(args2[src_index].AsFloat());
src_index++;
}

If we adopted IanB's suggestion of changing ScriptFunction instead, the array case does not arise.

[IanB]

@SEt,

Changing args2[src_index].IsInt() to args2[src_index].Defined() means that a redundant float to float copy will happen, which probably does not matter.

@Gavino,

Likewise your patch probably should also be checking for Ints in Float drag, although the Undefined case should not happen there.

@All,

One downside of this change is that currently in mixed Int-Float script arithmetic, Ints in Float drag retain the full precision of their Int-ness going into the expression. i.e float(268435457 - 268435456) = 1.0 where as float(268435457) - float(268435456) = 0.0 or perhaps it might be 16.0 depending on the exact values in use.

I am not say this should not be fixed, just that it will need banner headlines when we do.

I have had a quick look at doing this fix in ScriptFunction:: and although it is slightly more work it is still trivial and it will have less impact overall.

[SEt]

@IanB,
Not really. Actually constructor from float is simpler and faster than copy constructor used there now.

I strongly suggest making plugins and script functions have the same behavior.