Announcement

Collapse
No announcement yet.

Limiting City placement/terrain specific cities

Collapse
X
 
  • Filter
  • Time
  • Show
Clear All
new posts

  • #31
    Originally posted by Martin Gühmann

    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.


    Of course for readibility you would only indent the code dependent on the if.
    From the looks of things, I think that means I SHOULD add braces, right? It looks like you said that without braces it will only execute the first else if and not the other ones...did I get that right?


    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.
    Does this mean the first brace after the BOOL has to be in #if statement or keep it before it and just create a space?


    Also I notice Unitdata.cpp has A LOT of white space between functions. Is that just wordpad or should I clean it up a bit (for practice?)


    thanks, I can almost see the finish line
    Formerly known as "E" on Apolyton

    See me at Civfanatics.com

    Comment


    • #32
      Originally posted by E
      From the looks of things, I think that means I SHOULD add braces, right? It looks like you said that without braces it will only execute the first else if and not the other ones...did I get that right?
      No, that's wrong, basicly it means you can leave it as it is. Whether an "else if" doesn't depent on the braces it depends on the fact whether the "if" statement or "else if" statement was true and therefore the following code within the braces or the very next statement were executed. An if can only controll one statement and if it should control more then one stetement you have to put all the following statements into a block, so that it can controll the block as a whole.

      Originally posted by E
      Does this mean the first brace after the BOOL has to be in #if statement or keep it before it and just create a space?
      No, it doesn't mean this. If you put this under the #if then you have also to put the last closing brace associated to the function into the block and of course you have to do it for the original code and for your code as well. And actual I just requested that you put the final return uder the preprocessor, because in the original code it is not a return FALSE; but a return TRUE;

      Originally posted by E
      Also I notice Unitdata.cpp has A LOT of white space between functions. Is that just wordpad or should I clean it up a bit (for practice?)
      Yes all the files by Activision have such a huge amount of white space in it, I guess this is caused by the comment removement. However this is Activision original code and every space, tab and new line belongs to it. If you change this we have a hard time to detect the differences, if we do a automatic search for instance with UltraEdit or other but free tools, because they only mark the line differences, well there are also better tools. But nevertheless white space modification is also a code modification.

      Talking about white space modification, the very first brace of the function, the very first brace of the function that you put outside of ACTIVISION_ORIGINAL is seperated in the original code by two new lines from the function parameter list and is at the beginning of the line.

      Originally posted by E
      thanks, I can almost see the finish line
      Yes, but we want a perfect victory. And in my opinion, readibility of code is also very imprortant, even if it might also be a matter of taste, how to this and that in particular but there are also some conventions.

      -Martin
      Civ2 military advisor: "No complaints, Sir!"

      Comment


      • #33
        Great! then I think I made all the necessary changes and will post them later. and readability is important, and if I'm learning I want to atleast learn the right way
        Formerly known as "E" on Apolyton

        See me at Civfanatics.com

        Comment


        • #34
          However you aren't at the end, yet.

          To assure that we are talking about the same code I post it here:

          Code:
          //----------------------------------------------------------------------------
          //
          // Name       : CanSettleOn by E
          //
          // Description: Adds additional check for a flag to see what terrain 
          //              types the unit can settle on
          //
          //----------------------------------------------------------------------------
          
          
          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 TRUE; 
          #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;
          	return FALSE;
          #endif
          }
          And for comparision the original code so that this is also clear.

          Code:
          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->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; 
          
              return TRUE; 
          }
          Now in your version, if I compile it with ACTIVISION_ORIGINAL defined, the function returns TRUE if the terrain requirements aren't met instead of returning FALSE, now the function return behaviour is undefined if the requirements are met. Again if ACTIVISION_ORIGINAL is defined then the precomiler strips all the fragments that aren't under the defined(ACTIVISION_ORIGINAL) condition, to illustrate what the preprocessor with your code does if ACTIVISION_ORIGINAL is defined the resulting code:

          Code:
          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->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 TRUE; 
          }
          And that is not the same code as above, you changed the return under the searching condition so that it returns TRUE, even if it was in the original code return FALSE. And you forget the final return.

          And of course to demostrate what the preprocessor does with the code if ACTIVISION_ORIGINAL is not defined, the code:

          Code:
          BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 
                {
          	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;
          	return FALSE;
          }
          And this piece of code is finally compiled if ACTIVISION_ORIGINAL is not defined, the other piece of code is ignored. First thing is that the first "else if" and its code block in this fragment is still indented by one two much. And actual I wonder why you changed it into an "else if", however in this case it doesn't matter, because the function is left anyway if code associated to one of these ifs or else ifs is executed. Within the block of the for loop you have a new line too much, better use a new line two seperate the code of the for loop from the following if-else ifs.

          And another thing I have overlooked is that you check whether the position has a city, IIRC this should be just accessing a bool and not a whole for loop with some iterations and multiple evaluations of a condition. Therefore the best is to check for a city first before the for loop.

          For readability you should seperate the final return from the rest of the code as well, so that we all can see it more easilly.

          And again the first open brace of the function, the one at the very top, wasn't indented in the original code, no extra spaces, and actual I didn't request reinsert the additional new line.

          And finally a request for the function description, please fill in all the rest of the fields like in the other functions: Name, Description, Parameter, Globals, Returns and Remark(s), of course if you need help to do this ask. And please even if Fromafar does this, don't seperate this desription by new lines from the function. Microsoft Visual C++ displays this information, when you hoover the mouse cursor over the function in other pieces of the code.

          -Martin
          Civ2 military advisor: "No complaints, Sir!"

          Comment


          • #35
            I'm embarrassed by putting true instead of false..(stupid brain)...

            Ok, I think I incorporated all changes and I think I understood your request about not doing like Fromafar, I'm assuming that means sticking it in-between lines of code and that you prefer that they appear before the code...right?
            Attached Files
            Formerly known as "E" on Apolyton

            See me at Civfanatics.com

            Comment


            • #36
              Well we are getting closer, but still we aren't at the end.

              Let's start with some nit-picking about the layout, and you thought I would need a lot of patience with you, but for me its seems rather the way around.

              1. In the original code the first line within the function buddy was indented by two spaces and one tab, well for me it would also be enough if it would be just one tab, at least so that all tree lines look at least so that they have the same indention:

              Code:
                   sint32 searching = TRUE;    
              	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
                	sint32 t = rec->GetSettleCityTypeIndex();
              The above is the code in your file, correct the indention of the first line.

              2. Another point is the layout of the second if statement in your code:

              Code:
              	if (t < 0) {
              		return FALSE;      
              	}
              	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
              			return FALSE;                               
              		}
              	if (g_theWorld->HasCity(pos)) 
              		return FALSE;
              The second if is not ok, it has now the same indention like the surounding ifs, but not the following two lines, both lines have a tab too much. (Be happy that I don't nit-picking for the white space I don't see, the one at the end of the lines. )

              3. Now let's come to the stuff that breaks your code:

              Code:
              	for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
              		if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
              			return TRUE;
              		}
              	}
              
              	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;
              After the for loop you continue with an else if, but there is no corresponding if, well you may think that this if is before the for loop where you moved, but then you forgot to turn the first else if into an if. Remember if you start with an if, the next conditioned expression may be an if again or an else if, but you can never start such a sequense with an else if. Even in normal language you cannot use else without refering to something else. Here in C++ and in slic as well its another condition.

              4. And now again to the original code:

              Code:
              	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
              You changed back the last but one return, but still you forgot the final return TRUE;

              For comparision the original code:

              Code:
              	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; 
              
                  return TRUE; 
              }
              And note it has the final return true.

              And now to the function description:

              Code:
              //----------------------------------------------------------------------------
              //
              // Name       : CanSettleOn by E
              //
              // Description: Adds additional check for a flag to see what terrain 
              //              types the unit can settle on
              //
              // Parameters : Settler		: the units that can settle
              //
              // Globals    : g_theWorld	: terrain properties database
              //
              // Returns    : sint32		: terrain index value
              //
              // Remark(s)  : Modders will define this in Unit.txt as CanSettleOn: X 
              //              
              //
              //----------------------------------------------------------------------------
              
              
              BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos)
              Actual I meant to remove the two lines between the comment block and the function head, so that VC displays the description whenever I hover the mouse cursor over the function in other pieces of the code.

              What's the name of the function? Definatly it isn't CanSettleOn that some kind of property the function now checks for.

              Into the description belongs the answer of the question: What does this function? Well it's nice that the function now checks for an additional flag, but that is not what it does, but rather how it does. This belongs rather into the remarks that it has been modified. And what has been done.

              Parameters of the function: None of the parameters is called Settlers, we have one called unit_type and one called pos. And now tell me what use they have, before you post the next files , because I think you should think about it first.

              Globals: Take a close look on the function and count the globals in it. You should find two globals in it. g_theWorld isn't a terrain properties database, it rather describes the world you are playing, the map, number of continents etc.

              Returns: The return type is a BOOL, and therefore something else than a terrain index value. So let's see whether you have understood what the function does. So write it down.

              Remarks: Here you can tell everything you like to tell, what you have done, what is the use of your new flag, how to use it, etc.

              OK, that should be all for now.

              -Martin
              Civ2 military advisor: "No complaints, Sir!"

              Comment


              • #37
                Okay, I'll try this...

                Code:
                //----------------------------------------------------------------------------
                //
                // Name       : UDUnitTypeCanSettle
                //
                // Description: checks unit properties to see if it can settle a city on a tile 
                //              
                //
                // Parameters : GetNumCanSettleOn()	: checks for terrain type that 
                //                                                                unit can settle
                //
                // Globals    : g_theUnitDB	: Unit properties database
                //
                // Returns    : BOOL		: returns TRUE if terrain is the same as 
                //                                                  CanSettleOn
                //
                // Remark(s)  : Modders will define this in Unit.txt as CanSettleOn: X 
                //              
                //----------------------------------------------------------------------------
                BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos)
                Formerly known as "E" on Apolyton

                See me at Civfanatics.com

                Comment


                • #38
                  OK let's start: Name is fine, also Description, even if I would formulate it differently. Parameters is wrong GetNumCanSettleOn() is something that the function uses, but nothing that is passed to the function. If you want to use a function somewhere in the code then you have to know, what are the inputs and what are the outputs (return values, filled variables passed by refference). And what these outputs represent. From outside I am just interested in what the function does and not how the function does it. If you want to talk about the new CanSettleOn flag use the remarks for it.

                  Now to answer the question what the parameters are, take a look on the line of code after the comment block, the parameters of the function can be found within the parentheses. The names of these parameters should be self-explanatory.

                  For Globals: You added the g_theUnitDB to the description, but in return you removed the other global, actual I just told you to correct its description, but not to remove it entirely.

                  For Returns: A BOOL is correct but the description is to restricted, it doesn't only return TRUE if your flag is set and the terrain is right. It's wider, the function name should, should sum it up.

                  Remarks: As you want to tell the people so much about you have done, you should extend it a little bit.

                  And one remark in general, I know there are people who continue with small letters after a collon, but I prefere it to continue with a capital. Of course there are exception for instance g_theUnitDB is a case sensitive name and therefore can't be changed. However you have to do a decision, either you use capitals after a colon or not. As you use a mixed style.

                  And if you have to insert linebreaks put the start of the next line under the start of the line before, like you did in your earlier description attemp. And another note is to use for these comment blocks spaces instead of tabs. Tabs don't have on every editor the same length, some uses eight spaces and others uses four spaces. I hope you switched to EditPlus 2 in the meantime.

                  And nice that you removed the two new lines.

                  -Martin
                  Civ2 military advisor: "No complaints, Sir!"

                  Comment


                  • #39
                    Here's to hope

                    Code:
                    //----------------------------------------------------------------------------
                    //
                    // Name       : 	UDUnitTypeCanSettle
                    //
                    // Description:	Checks unit properties to see if it can settle a city on a tile
                    //
                    // Parameters :	sint32 unit_type	: Variable for the type of unit 
                    //			const MapPoint &pos	: Variable for tile on map
                    //
                    // Globals    :	g_theWorld		: Terrain properties database
                    //			g_theUnitDB   	: Unit properties DB
                    //
                    // Returns    :	BOOL			: Returns TRUE if an unit-type can Settle on a tile
                    //						  FALSE if the unit cannot settle a city there
                    //
                    // Remark(s)  :	Enables a new unit attribute CanSettleOn.
                    //			Modders will define this in Unit.txt as CanSettleOn: X.  
                    //			THe flag adds an additional option in order to restrict where cities can be built. 
                    //              
                    //
                    //----------------------------------------------------------------------------
                    Formerly known as "E" on Apolyton

                    See me at Civfanatics.com

                    Comment


                    • #40
                      Hi think we have it now approximatly.

                      One point to the lay out: Better use spaces instead of tabs in such a comment block, here we have a ceratin layout that is messed up if you change the indent a tab represents. For instance you can select in EditPlus2 whether it should display the tabs four spaces or eight spaces long. Eight spaces is the default setting for new created syntaces, and four is the setting for C++ code.

                      Parameters look all right now, even if I would describe it differently, but it seems you have understood that.

                      For globals: g_theWorld is actual much more than a terrain property database, actual it contains data about the map, each tile, the world rules, etc. So better call it: The game world properties. I think I should avoid the word database in this case. But it is right that it is not easy to describe it. And therefore we see some too limited descriptions for it in the code.

                      The Returns description seems also OK, in particular I liked the layout in your post, so that the FALSE starts in the same column directly under the TRUE.

                      And for the Remark(s): The second and the third line are OK, but I would be more specific in the first line, namly that it instead enables a new unit attribute, a new unit atrribute was added, and in particular by you. So that we have the piece of information about the author of the addition there.

                      -Martin
                      Civ2 military advisor: "No complaints, Sir!"

                      Comment


                      • #41
                        Thanks Martin for all your help. HopefullY I'm not so difficult next time
                        Formerly known as "E" on Apolyton

                        See me at Civfanatics.com

                        Comment


                        • #42
                          You aren't quite finished. However the code in UnitData.cpp is now fine, except that I would add at least a tab in the Orignal version of the code in the line: "sint32 searching = TRUE;" And maybe you should replace all the remaining tabs in the function description by spaces. Depending on the tab length it may look different in each editor. And maybe you should take a look again on the description at the start of the file you added.

                          Well actual the real problem with the files you have posted is that I cannot find CanSettleOn in unit.cdb that means your code doesn't compile at all.

                          -Martin
                          Attached Files
                          Civ2 military advisor: "No complaints, Sir!"

                          Comment


                          • #43
                            thanks Martin,
                            my error in th cdb is that I had it as Cant instead of Can

                            ANd I did change everything to spaces now
                            Attached Files
                            Formerly known as "E" on Apolyton

                            See me at Civfanatics.com

                            Comment


                            • #44
                              I didn't check whether it compiles as I don't have the time to do it, but it seems to be finished. However I would give the BOOL an indent of two spaces, like the globals have:

                              Code:
                              // Globals    :  g_theWorld             : The game world properties
                              //               g_theUnitDB            : Unit properties 
                              //
                              // Returns    :BOOL                     : Returns TRUE if an unit-type can Settle on a tile
                              However this is up to you whether you change it. So go to your next project.

                              -Martin
                              Civ2 military advisor: "No complaints, Sir!"

                              Comment


                              • #45
                                Thanks Martin, I will make the change in the altered files...
                                Formerly known as "E" on Apolyton

                                See me at Civfanatics.com

                                Comment

                                Working...
                                X