Limiting City placement/terrain specific cities

[Martin Gühmann]

Originally posted by E
Am I getting closer? and I didn't see any other references to Settle and CantBuildOn so I think this is the only place in the code to do this.

Yes you are getting closer, but still you aren't at the end. The code still doesn't work, the code you have added still belongs into a for loop and now I think this should be enough, however the code should still be optimized by removing this searching variable.

-Martin

[Ekmek]

Thanks martin,
this is the change I added to remove the searching function. Not sure where else to go in regards to the for loop. But you did say this enough so this is the code I've added. I'll attach it here until you say its worthy of going into altered files...

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
	}


	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == FALSE) {
		return FALSE; 
	}

	if (g_theWorld->HasCity(pos)) 
		return FALSE; 
      if(rec->GetCanSettleOnIndex() == cell->GetTerrain()) { //added by E
		return TRUE;
	 }
	if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		searching = FALSE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		searching = FALSE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		searching = FALSE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		searching = FALSE; 
     
	//if (searching)   --->E add notation to remove searching function recommend by Martin G 
	//return FALSE;    --->E add notation to remove searching function recommend by Martin G

    return TRUE; 
}

[Martin Gühmann]

This still doesn't work I said that you should insert you initial for loop at the place where you inserted the code fragment the time before.

Next thing seraching is not a function it is a variable of type sint32 that is that initially to TRUE, as far as I know True is a #define and with the value 1.

This searching variable is still in the function, actual I asked you to comment out every instance of it. And I asked you to do another thing, namly to insert everywhere a return TRUE; where it was originally set to FALSE.

-Martin

[Ekmek]

Ok, I added back in the for code that I had before, and removed my simple coded and commented out the searching stuff. Does the for statement work now? Do I need to define the variable "i" ?
I think I'm understanding your responses but thanks for your help. slowly but surely I'm learning.

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	//sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		//return FALSE; 
                               return TRUE;
	}

for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
			if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
				return TRUE;


	else if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
		//return FALSE; 
                               return TRUE;
	}

	else if (g_theWorld->HasCity(pos)) 
		//return FALSE;
                               return TRUE; 
      
	 }
	else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		//searching = FALSE;
                                 return TRUE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		//searching = FALSE;
                               return TRUE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		//searching = FALSE;
                                return TRUE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		//searching = FALSE; 
                                return TRUE;
     
	//if (searching)    
	//return FALSE;   

    return TRUE; 
}

[Martin Gühmann]

Originally posted by E
Ok, I added back in the for code that I had before,

You added back the for loop but you should add it right before the code that checks in the original version whether the unit has the right properties for instance whether it can settle on and its current position is land.

Originally posted by E
and removed my simple coded and commented out the searching stuff.

You removed it and a lot more, now the function returns always TRUE no matter what you do. You should have replaced everytime if the searching variable was set to FALSE by return TRUE; I never talked about any return FALSE; statements. And there is another thing I think I didn't talked about yet. The last return TRUE; in the function must become a return FALSE; sice we have removed the searching variable.

Originally posted by E
Does the for statement work now?

No, it still doesn't work, it even doesn't compile sice you have forgotten to close the two blocks that were opened by an open brace '{'. Of course you do this by adding closing braces '}'. One for closing the if block after the return and another one to close the block of the for loop afterwards you have closed the if block.

Originally posted by E
Do I need to define the variable "i" ?

Of it needs to be defined before, since the i should be just a counting variable, you can do it in the same way like the searching variable, and the same place where it was defined at the top of the function, but you can also define it right before the for loop. Actual this is the way to do it if you have something else then a primitive type or a pointer.

-Martin

[Ekmek]

ok I think I got it, but maybe I left something out...

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	//sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
                
	}


	else if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
		return FALSE; 
                               
	}

       for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
			if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) }
				return TRUE;

	else if (g_theWorld->HasCity(pos)) 
		return FALSE;
        else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		//searching = FALSE;
                                 return TRUE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		//searching = FALSE;
                               return TRUE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		//searching = FALSE;
                                return TRUE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		//searching = FALSE; 
                                return TRUE;
     
	//if (searching)    
	//return FALSE;   

    return FALSE; 
}

[Martin Gühmann]

Originally posted by E
ok I think I got it, but maybe I left something out...

Yes we are getting closer.

You forgot to define the i.

And instead of closing the two {}-blocks you have opened in the for loop you replaced the second open brace by a closing brace.

So again the return TRUE; under the for loop belongs into the if block of the for loop and of course this if block needs also to be encapsulated. So the foor loop opens a {}-block and the following if is within this block that opens also a {}-block, that contains a return TRUE;

And another problem you cannot continue after a for loop with an else if there is no coresponding if, so turn the else if right after the for loop into a simple if.

-Martin

[Ekmek]

Thanks Martin. Hope your patience isnt getting exhausted but if its not too time consuming maybe your work with me and bigMC can sort of make up how short we are on programmers

I think I got it, now, I'll post my file after I get a satisfactory grade from the teacher and hopefully my next homework goes better (I did learn a lot thanks)

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	//sint32 searching = TRUE;  
                sint32 i;
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
                
	}


	else if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
		return FALSE; 
                               
	}

       for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
			if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
			return TRUE;
       }
}

	if (g_theWorld->HasCity(pos)) 
		return FALSE;
        else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		//searching = FALSE;
                                 return TRUE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		//searching = FALSE;
                               return TRUE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		//searching = FALSE;
                                return TRUE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		//searching = FALSE; 
                                return TRUE;
     
	//if (searching)    
	//return FALSE;   

    return FALSE; 
}

[Martin Gühmann]

Looks compilable now, and it should do what you indend to do. So let's come to the aesthetical part and clean up the lay out. C++ is a layout free language, that means it doesn't matter how much white space (spaces, new lines, tabs) you insert into the code or how less. But for a human reader it does mater for the sake of readibility.

Well as our server is still not running properly I would continue with the ACTIVISION_ORIGINAL stuff.

So copy the original code, to the above code in the souce file. Mark the beginning of the old code with

#if defined(ACTIVISION_ORIGINAL)

Seperate the new code with the preprocessor derective

#else

And mark the end of the new code with

#endif

Look into the code for examples for this.

Now cleanup your code the easiest thing is to remove the outcommented bits of old code.

Now to the next step: idention

Whenever you open a block with a brace '{' use one tab to indent the following code. However a certain number of spaces also do the job, that is actual a matter of preference, however I would do it like it is done in the rest of the file. And there we have tabs.

Here is a code fragment for example:

if(something){
	//Indent the inside code by one tab
	for(i = 0; i < something_else; ++i){
		//Indent the inside code by one tab again
	}
}

The most important thing is that you see on a glance which piece of code belongs into which block, and you can also see whether you have forgotten to close a block by a closing brace '}'.

I prefer a compact style, no space between the if or for and the following open parenthesis '('. And I put the brace that follows the if or for onto the same line, and without spaces as well. In another style the following brace is put on a new line. And the following code is put on another new line. I don't like this style, because it adds so much lines without any information, maybe accept that you can find the matching pairs of braces better. But that is no gain if I see then just half of the code on one glance.

What you have to do now is to remove some tabs at one place and add some at another place, so that it looks well. And some new lines should be removed, as they don't make the code clearer. Of course the ar also new lines that makes the code clearer. But I guess this lies in the eye of the beholder.

And another little request, could you post the additions to the unit.cdb, just to check whether everything is alright there.

-Martin

[Ekmek]

Martin,
Thanks. I'll start on my clean up in the mean time here is what I put in unit.cdb

Record Advance             EnableAdvance
	Record Advance[0..5]        ObsoleteAdvance
	Record Government[] GovernmentsModified
	[b]Record Terrain[] CantSettleOn //added by E[/b]

	Bits Size {
		Small,
		Medium,
		Large
	}

[Ekmek]

Martin,

Here are the files so you can check my formatting. Thanks for the help!

[Martin Gühmann]

First it is not the same code you have posted above and the format looks terrible.

If I define ACTIVISION_ORIGINAL, it doesn't generate the original code at least the code does the same. But if I don't define it, it doesn't matter anymore whether the settle unit has pop and can build or not.

And the formating is a mixture of tabs and spaces. So again to do the ACTIVISION_ORIGINAL part, do soemthing like this:

#if defined(ACTIVISION_ORIGINAL)
	 //Original code by Activision, does not contain 
	 //any changes, and no format changes, either.
#else
	 //Your code including all the alteration, 
	 //since you modified, the half function you
	 //you put the functions old and new into the
	 //the according blocks as a whole. 
#endif

The next step is to remove all these spaces from your code now, and replace them by tabs (the key left of the Q) if necessary. And again if you open a block with a brace the following comes into a new line with one tab of indention. If you close a block you close it with a closing brace, and that brace should get a new line and should have the same indetion like the if, for, etc. that opened its block, so that you can easily see, which closing brace, belongs to which if. Then else if and else, occur never without an if, therefore they get the same level of indetion like the coresponding if so that you can see which else or else if belongs to which if.

-Martin

[Ekmek]

Ok I gave formatting another go, but I did notice that the activision 'If-else' statements have braces around their 'return false' but they did not do it with the later 'SettleLand' stuff. Should I added braces there or is it not necessary?

Also I'm editing in WordPad, is there a better program to use (Notepad was a problem sometimes)

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 
      {
#if defined(ACTIVISION_ORIGINAL)
     sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
	}

	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == FALSE) {
		return FALSE; 
	}

	if (g_theWorld->HasCity(pos)) 
		return FALSE; 

	if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		searching = FALSE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		searching = FALSE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		searching = FALSE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		searching = FALSE; 
     
	if (searching) 
		return FALSE; 
#else
	sint32 i;
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
	sint32 t = rec->GetSettleCityTypeIndex();
	if (t < 0) {
		return FALSE;      
	}
		else if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
			return FALSE;                               
		}
	for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
		if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
			return TRUE;
		}
	}
	if (g_theWorld->HasCity(pos)) 
		return FALSE;
		else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
			return TRUE; 
		else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
			return TRUE; 
		else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
			return TRUE; 
		else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
			return TRUE;
#endif
	return FALSE;
}

[Ekmek]

and the files

[Martin Gühmann]

At first, looks better now, but still we aren't at the end.

Originally posted by E
Ok I gave formatting another go, but I did notice that the activision 'If-else' statements have braces around their 'return false' but they did not do it with the later 'SettleLand' stuff. Should I added braces there or is it not necessary?

That's a good question, and in fact the is a difference. Everything within the braces following the if are under the condition of the if. If the following is not included within braces, only the statement directly following is conditioned by the if.

int i = 8;
if(something){
	i = i + 1;
	i++;
	--i;
	i+=2;
}

If something == false the value of i is at the end after the last statement 8, because nothing in the if block was executed.

int i = 8;
if(something)
	i = i + 1;
	i++;
	--i;
	i+=2;

In this case i equal 10, only the statement right after the if is not executed if something equal false.

Of course for readibility you would only indent the code dependent on the if.

int i = 8;
if(something)
	i = i + 1;
i++;
--i;
i+=2;

Originally posted by E
Also I'm editing in WordPad, is there a better program to use (Notepad was a problem sometimes)

What about EditPlus 2, it has syntax highlighting and Locutus has a set of syntax files for slic, and there should also be some updated ones here around in the forum.

Now let's come to the layout, the first else if statement is indented in comparison to its coresponding if. Actual the else if code is executed if the condition of the if is false, and therefore they are on the same level and get equal indention.

For the sake of readibility I also would sourround the for loop in the middle with new lines. However this is really a matter of prefference and taste.

And the first brace was in the original code on the line start. And should also be there if you give it a new line like in the rest of the file.

And now to the ACTIVISION_ORIGINAL statement, the old function retuned TRUE, if the execution came to the last statement, not return FALSE as it is now even if you define ACTIVISION_ORIGINAL. However if you put the preprocessor derectives into the function, then you have to include the last return as you have changed it.

-Martin