[Freeciv-Dev] Re: (PR#14443) city walls not visible (effects problem)

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=14443 >

Per I. Mathisen wrote:
> <URL: http://bugs.freeciv.org/Ticket/Display.html?id=14443 >
> 
> On Thu, 17 Nov 2005, Jason Short wrote:
> 
>>A big part of the ugliness is it's difficult to tell where the problem
>>is even going to crop up.If you do a wrong query it can happen.  But
>>how are you supposed to know what queries are right?It's in the
>>documentation, but there is currently no compile-time or run-time
>>detection of errors.Run-time detection would be easy enough (though
>>tedious) if each effect type told which query function it was to be used
>>with.However there may be a more elegant way to merge query
>>functions...or to add a "needs_unit" flag to an effect type to show it
>>can only be used with a query function providing a unit.
> 
> 
> Run-time detection is necessary.

Agreed.

>>2.Use the attached patch that changes the way unit* req queries work.
>> In this patch if no unit is given the req returns TRUE not FALSE.This
>>is basically a hack to get around the problem (the problem is such
>>queries should not be called at all).However it would solve the
>>city-walls problem, potentially with unforseen side effects.One
>>forseen side effect is that coastal defense and SAM batteries will be
>>drawn as city walls.
> 
> I think this solution is fine. We should be changing the way coastal and
> SAM work anyway.

I do not think it is fine...it is a hack that has side effects that will 
eventually bite us.  But it could be an acceptable workaround.

>>3.Change city_got_citywalls to check building presence not effect
>>presence.This is almost certainly a good idea anyway.  However it
>>still doesn't solve the underlying problem.
> 
> How would this not be a regression to B_WALL and B_CITY that we have
> worked so hard to leave behind?

Ideally city_got_citywalls is removed entirely. Individual callers then 
do direct checks on what it is they are actually checking for.  In the 
client, this will amount to building checks: the tileset code will be 
able to draw any building if graphics are added for it, and the text.c 
code will probably need some additional building flag.  In the AI code, 
it amounts to checking for effect presence directly.

As I said this is almost certainly a good idea anyway; in a generalized 
effects design "citywalls" has no particularly clear meaning.  But...it 
won't solve the problem for other effect types.

>>4.We could add checks inside the reqs (NOT effects) code directly...if
>>a reqs check is done and the target is not given, we could crash or
>>print an error.However this might have unforseen side effects as well
>>since it's not clear to me that these queries are /always/ invalid.
> 
> I do not understand this one.

It amounts to using the patch from #2 but changing the NULL case to an 
assertion.  Probably not a good idea.

-jason