[Freeciv-Dev] Re: PATCH: rand_pos function and usage(PR#1017)

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 01:19 AM 01/10/24 -0400, Jason Dorje Short wrote:
>"Ross W. Wetmore" wrote:
>
><snip context: RAND_POS_CHECKED macro>
>
>> This clearly significantly increases the computation effort,
>
>Not necessarily.  In a mostly-grassland map the overhead isn't necessary,
>but in a highly-forested or highly-desert map the fact that the loops are
>bounded would be a big advantage.

You missed the point. The example explaining it is in another email.

BTW: It is bad that you missed the point. It shows you don't understand 
     the practical aspects/needs.

>> introduces
>> a lot of almost impossible to solve issues relating to the size of the
>> allocated arrays vs the selection space and final selection sixe,
>
>It uses a fair amount of memory.  This memory is freed shortly afterwards,
>and shouldn't be a problem overall.

No, your initial algorithm mis-used memory. Not understanding the difference
between total selection size, successful selection size and actual size
or number of elements to return would lead it to all kinds of problems,
incompleteness or difficult to track bugs.

The last version got rid of many of these issues, but doesn't do anything
worthwhile either.

>> The interface is also so incredibly unwieldy and limiting that you
>> really can't do any significant filter operations.
>
>It is certainly ugly.  If we follow Raimar's idea of having actions within
>the loop, it will be even more ugly (although more powerful).

Ugliness has nothing to do with it. It is practically unwieldy and
limiting which are real problems rather than prettiness problems.

>> I'm not sure why anyone would waste time trying such things :-).
>> 
>> There are better and higher priority things to do to fix freeciv.
>
>Unfortunately, some of them depend on this.  I would like to make mapgen.c
>topology-independent.  This is fairly easy to do (I've already provided a
>full patch to do it), but until we agree on how to do it we'll still be
>stuck here and no new topologies will be possible.

Nothing currently depends on this and nothing needs to depend on this.

The problems are all being created by you on the fly :-)

BTW: There is no problem running the mapgen.c replacement in any of the
     4 rectangular topologies, and in corecleanup_07 the existing mapgen
     had minimal fixes to do the same.

>Note that I'm not asking that mapgen provide good maps for every topology. 

I know, you trying to make it provide bad maps for every topology :-).

It is better to just leave it alone, or apply available fixes to make 
good maps in the standard case and be able to use them in any of the 
other topologies until someone creates a mapgenerator that does 
specialized good maps in the case of other topologies.

The latter can really wait until you get the first "n" steps of the 
topology work completed.

>I require only two things: that the map generation not be any worse for the
>current topology, and that it is able to make a map (any map) for an
>arbitrary topology.  I do not particularly care about speed or memory usage,
>since such things shouldn't be a problem during map generation anyway.

Your speed criteria seem to be always inverted to the norm.

The initial 2 conditions are best met by leaving the current code alone
since it already satisfies them for the cases of concern.

>Your corecleanup patch doesn't touch on mapgen.c so I have no other work to
>go by here.  How would you change mapgen to accomplish the above two goals? 

They are really more or less there. But if you need more ...

You can apply the mapgen.c replacement that has been in all the corecleanups.
It might even work with CVS if DIR_CCW etc. is used in the places where it
does really need to use rotational map_steps. This is pretty much the river
code. Some form of block_iterate is also needed. But both of these can be 
put in as local macro fixes/hacks until the general CVS codebase reaches the 
necessary level of capability. The initial corecleanup versions actually ran
this way, i.e. with self-contined corecleanups.

One added plus is this would fix a lot of the spurious bugs in selecting 
start positions and the like - it uses "filtered" selections. 

If some maintainer commits to a proper review of the mapgen part of the 
corecleanups which includes a commitment to deal with real changes, and 
not just pick and choose 2 or 3 minor cosmetic sub-patches amounting to a
noise level like 5% or less while totally ignoring the rest, then I will 
cleanup and resubmit the mapgen patch for current CVS.

<Aside>
I view the current generated maps as being of unplayable quality with 
few realistic geographical features and the codebase as only slightly
better than Syela core code. I spent considerable time cleaning algorithms,
adding features and fixing various bugs. There is not much point from my
perspective to start this all over again from scratch. However, building
on this new base or at least significant portions of it, is definitely
an area I consider worth spending my time on, so there is dicker room.

Mapgen is also interesting in that it is drop in replaceable, used in 
the initial setup, but not afterwards in the code. Thus there is little
risk from code changes, but has a user noticeable "feature" impact in the 
resulting game play. Practical aesthetics are important. 

Anyone who starts making changes, needs to understand there will be a lot 
of noise, so prior commitment and general consent is a precondition.
</Aside>

>Any solution is OK with me.
>
>jason

I would also take responsibility for any bugfixes to keep these standard
map generators running through the topology changes, and produce a new
generator 
to handle general topology features agreed upon in some collective proposal.
I actually have a strong interest in playing on good maps as opposed to the
stuff that is currently produced, so would do some of this on my own anyway.

But I doubt Raimar or the other maintainers are quite ready for either of
these levels of activity yet :-).

Cheers,
RossW
=====