[Freeciv-Dev] Re: (PR#4729) cleanup to canvas<->city pos functions

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

Jason Short wrote:
> rwetmore@xxxxxxxxxxxx wrote:
> 
>>Worthwhile, but fix the one inconsistency which is actually I think a
>>future raft of bugs in hiding.
> 
>>>(Note city_to_canvas_pos could return a boolean, for consistency, but it 
>>>should always be TRUE.  We should never have to deal with off-canvas 
>>>tiles at all.)
>>
>>bad (for three reasons besides the final foolish wishlist sentence)
>>
>>1)  It is not always TRUE, and/or forcing coders to insure this condition
>>leads to all sorts of ad hoc ways to check or miss the checks.
>>
>>2)  All the coordinate functions should return a standard boolean that
>>confirms that the transformation was successful (even if it seems like it
>>cannot fail). Thus one never has to worry about which does and doesn't.
>>and never has to update code when a future update adds something that
>>changes this. Think about interfaces, that means thinking about the future
>>and not just the present or past.
>>
>>3)  It would not hurt to return the condition of tile_visible_map_canvas()
>>for city_to_canvas_pos(), or the equivalent clip part of map_to_canvas_pos()
>>since it is perfectly reasonable for one to iterate over city coordinates in
>>a canvas setting without doing either a checked iteration (to guarantee any
>>city tile is on the map, and a city can overlap the map edge), or that it
>>lies within the current gui window. One can always ignore the boolean, but
>>it would seem like it is more often useful than not and should be a quick
>>test.
>>     In fact, I would probably return the clip() condition which is valid in
>>all contexts of this transformation, and make a second
>>city_to_canvas_pos_checked() that calls this and adds the map condition.
>>This is much the same as the city iterators that can run in full city or
>>map constrained modes.
> 
> 
> So you're saying that city_to_canvas_pos should return the boolean?  I 

Transformations should always ... (with the one glaring exception that makes
the rule valid :-).

> can't quite understand the syntax of your response (is it the note or 
> the patch that is "bad")?
> 
> jason

I think your confusion is because there are two sets of functions like in
the two sets of city macros and an inherent ambiguity in saying "canvas".

In the cases you showed, you are converting to a city_canvas rather than
a map_canvas with assumptions about the origin relationships. There is no 
origin offset here and thus 4 args. The clip() for the canvas window is
probably always exactly that for the city modulo the corners. This is a
very rigid transformation == lots of (premature) optimization.

Like in city_iterate_checked, where you introduce a canvas to map origin
relationship concept beyond the simple city_iterate and two extra args,
there is a set of functions that are more general in that they can be used
in a map context and check for unreal coordinates, not just clipped ones.

There are uses for both with one being an extension of the other.

Again, though, I would put the map flavours in map.{c,h} and not city,{c,h}
even though they bridge the two coordinates, as the map/unreal condition is
more of the extension, and more likely to be used in map code.

Cheers,
RossW
=====