graphic points

[mwieder]

I just pushed a change to allow non-integer point specifications for graphics (in util.cpp). This will allow us not to have to think about truncating calculated point locations in scripts, etc. Before I issue a pull request, are there drawbacks to this? So far it seems to work as expected.

[monte]

Sounds good to me! Are you truncating or rounding is my main question.

[mwieder]

Just truncating. Seemed like a coin toss.

Update: actually I'm truncating by way of casting to (int). Took the easy way out.

[monte]

Hmm... you're more likely to want the point at the closet position though... I think rect allows reals too... what method does it use?

[mwieder]

Heh. It was hard enough to chase down the point stuff. Don't know what rects use at the moment.
Rounding shouldn't be a problem, although there might be a performance hit.

[mwieder]

OK - rounding added. I'm not rounding negative points properly, should be rounded the other way around, but AFAICT for negative coordinates that shouldn't matter, i.e., they'll be one pixel off but they're offscreen at the time.

[LCMark]

I just pushed a change to allow non-integer point specifications for graphics (in util.cpp). This will allow us not to have to think about truncating calculated point locations in scripts, etc. Before I issue a pull request, are there drawbacks to this? So far it seems to work as expected.

The only drawback I see is that of setting expectations inappropriately. At the moment if you pass non-integer points to the graphic object it will complain and thus it prompts you to realize that it only supports integer co-ordinates...

Hmm... you're more likely to want the point at the closet position though... I think rect allows reals too... what method does it use?

It uses MCU_strtol - which accepts real numbers if the 'reals' parameter is true. Indeed, the only change to bring the same behavior to the points property of graphics is to change the MCU_strotl calls in MCU_parsepoints() to pass True for that parameter. The question though, is whether this is 'correct'!

Rounding shouldn't be a problem, although there might be a performance hit.

If there was it would be infinitesimal in comparison to the work of actually setting the property as a whole

OK - rounding added. I'm not rounding negative points properly, should be rounded the other way around, but AFAICT for negative coordinates that shouldn't matter, i.e., they'll be one pixel off but they're offscreen at the time.

I think it does matter even if they are offscreen as the control could be moved on screen after the points are set (e.g. by setting the loc of the graphic).

Now, of course, really the question here is what the behavior should be - it is important to get this right as if we are going to make the change it should be consistent with when we add features that do use sub-pixel positioning. As mentioned above the rect property uses MCU_strtol which does a truncation - essentially round-to-zero. However, (without having thought about it too much) I think this is probably wrong and things should be rounded down...

[mwieder]

At the moment if you pass non-integer points to the graphic object it will complain and thus it prompts you to realize that it only supports integer co-ordinates...

No, actually at the moment it totally screws up the graphic.

It uses MCU_strtol - which accepts real numbers if the 'reals' parameter is true. Indeed, the only change to bring the same behavior to the points property of graphics is to change the MCU_strotl calls in MCU_parsepoints() to pass True for that parameter.

I changed that in MCU_parsepoint as well. But there's one more change that needs to be made in MCU_strtol itself in order for the parser to accept non-integer values, otherwise it just ignores the decimal point and anything afterwards.

I think it does matter even if they are offscreen as the control could be moved on screen after the points are set (e.g. by setting the loc of the graphic).

But...that changes the points of the graphic anyway. At any rate, it's a simple matter to round negative values in the other direction. Just seemed like extra complexity if it wasn't really needed. I'll add that as well this evening before issuing a pull request.

when we add features that do use sub-pixel positioning

Ah. Maybe that's on the timeline somewhere. But for now this fixes a problem that's been annoying since the beginning of MetaCard and a source of much confusion to anyone coming into the language.

truncation... should be rounded down

I don't see the difference.

[LCMark]

No, actually at the moment it totally screws up the graphic.

Okay, that was just wishful thinking on my part - I'll rephrase - 'I *thought* it complained...'.

I changed that in MCU_parsepoint as well. But there's one more change that needs to be made in MCU_strtol itself in order for the parser to accept non-integer values, otherwise it just ignores the decimal point and anything afterwards.

The only issue with changing MCU_strtol is that it changes every use of it - if it is ignoring things after the decimal point then it is essentially rounding to zero. There might be good reasons why this is how it is coded - and without looking at every use of MCU_strtol, I couldn't tell you what the impact would be.

But...that changes the points of the graphic anyway. At any rate, it's a simple matter to round negative values in the other direction. Just seemed like extra complexity if it wasn't really needed. I'll add that as well this evening before issuing a pull request.

I don't see the difference.

Let's say you have a set of points (all reals) in a list, and some are negative (but not less than -10). The the following invariant is probably a good one to have:

Code: Select all

   set the points of tGraphic1 to tMyRealPoints
   ... add 10,10 to each of the points in tMyRealPoints ...
   set the points of tGraphic2 to MyRealPoints
   ... the points of tGraphic2 should be +10,+10 to the points in tGraphic1 ...

Ah. Maybe that's on the timeline somewhere. But for now this fixes a problem that's been annoying since the beginning of MetaCard and a source of much confusion to anyone coming into the language.

The problem that it messes up the graphic now? Or the problem that it doesn't throw an error informing you what the structure of the points should be? To be fair, since the rect property already does some sort of truncation/rounding (perhaps not correctly), I'm not going to argue that 'the points' of the graphic shouldn't do a similar thing. I do think it's important to get the type of rounding right in both cases though

[mwieder]

The only issue with changing MCU_strtol is that it changes every use of it - if it is ignoring things after the decimal point then it is essentially rounding to zero. There might be good reasons why this is how it is coded - and without looking at every use of MCU_strtol, I couldn't tell you what the impact would be.

Yes, that's exactly why I brought it up here. So far in my testing everything seems to work, but I haven't looked for all the instances of MCU_strtol.

Re: "I don't see a difference..." you said "rounded down" as opposed to "truncated". Your example with mixed positive and negative values is a good answer. I'll probably just throw a round() macro in the code to keep it readable.

[LCMark]

Re: "I don't see a difference..." you said "rounded down" as opposed to "truncated". Your example with mixed positive and negative values is a good answer. I'll probably just throw a round() macro in the code to keep it readable.

Truncation is equivalent to round-to-zero. i.e. -1.5 -> -1, 1.5 -> 1.
By rounding down, I really meant round-towards-negative-infinity. i.e. -1.5 -> -2, 1.5 -> 1.

[mwieder]

By rounding down, I really meant round-towards-negative-infinity. i.e. -1.5 -> -2, 1.5 -> 1.

Hmmm... well,I'm not convinced that's correct.
I think the proper form of point rounding should be

Code: Select all

(int) (x > 0 ? (x+0.5) : (x-0.5))

[mwieder]

MCU_strtol:

As currently written, MCU_strtol returns an integer value unless a real number is explicitly requested. This request is only made in two files: util.cpp and mblcontrol.cpp. In the cases where a real number is requested (by setting "reals" to True) MCU_strtol *still* returns an integer because it ignores the decimal point and anything following. A two-line change to the code fixes this.

Fixing it has repercussions in MCNativeControl::ParseRange, which currently requests a real and receives an integer. I don't know what effect returning the requested real value will have. I have two guesses: either it will continue to work, or it should be requesting an integer value.

This also affects the MCU_stoi2, MCU_stoi2x2, and MCU_stoi4x4 functions in util.cpp. They're called from various places and do the same thing as above: request a real number value and receive a truncated version. I haven't yet gone through all the use cases, and I'll put more work into this, but it's working so far in things like (in operator.cpp)

Code: Select all

put "3.1416, 2" is a point
-- note that without the fix the comparison is to "3,2"
put "1.1,2.2,3.3,4.4" is a rect
-- note that without the fix the comparison is to "1,2,3,4"

[LCMark]

As currently written, MCU_strtol returns an integer value unless a real number is explicitly requested. This request is only made in two files: util.cpp and mblcontrol.cpp. In the cases where a real number is requested (by setting "reals" to True) MCU_strtol *still* returns an integer because it ignores the decimal point and anything following. A two-line change to the code fixes this.

Looking at the implementation of MCU_strtol,the intent is that if you pass 'reals == True', then it should accept a real, but just return the integer part (i.e. round towards zero).

Indeed, if 'reals == False', and it tries to parse a real, then it returns 0 and done remains False. If 'reals == True', and it tries to pass a real, then it returns the integer part and sets 'done' to True.

Thus the purpose of MCU_strtol is never to return a real and the 'reals' parameter is just to say that if a real is given it should return the integer part.

For example, the 'flaggedRanges' property takes a sequence of integer pairs - these really should be integers and throw an error if it is given a real. (Indeed, it doesn't set reals == True - so they will error out on non-integers).

Then there are cases like 'the repeatCount' property which uses MCU_stoi2(). Now, again, it doesn't make any sense for this to accept a real as far as I can see, but it will accept one and use the truncated result.

Now, the refactored branch has tightened / sorted this out to a certain degree since the uses of singles, pairs and quads of 'integers' (i.e. those parsed by MCU_stoi*) have been made distinct (i.e. if a method wants an MCPoint it asks for one).

So really I think, this comes down to two things at the moment:

Making P_POINTS not broken - this could either be done by making it throw an error if MCU_parsepoints fails (which it does if it has any reals in it at the moment - the caller just doesn't check); or by accepting reals and rounding them appropriately.

Making the rounding used in MCU_strtol more consistent with ep.getint4() - i.e. round to nearest, rather than round to zero.

[LCMark]

Btw, we had a mid-air collision in posting it seems, and my response to your previous post got lost... But, for the record:

Hmmm... well,I'm not convinced that's correct.

Neither am I...

I think the proper form of point rounding should be... <insert code for round to nearest>

Yes - I agree - not just for points, but generally as that is what 'ep.getint4()' and friends do. i.e. If the engine is going to accept reals and round them to integers anywhere, it should do so with consistent rounding mode everywhere.