UnitAttachment Which Allows/Requires Another Unit To Move

Cernel

@cernel I meant still somehow supporting the "OR" (in a logical / intuitive way), of course, and the current functionalities or whatever you two want to have. I guess you need both the "OR" and "AND"? Like you also need having the "rail" plus something else, as well, to move the "train" through? Just wondering if both are needed for your user case.

Hepps

@cernel To be honest cernel... I tried to make the suggestion as simple as possible in order to make it achievable without complicating its design.

The reason I excluded any kind of "count" system in the original proposal was because it seemed to me that it would...

a) Make its integration into the code far more difficult. Obviously a big assumption on my part since I know nothing of the code... but an assumption made based on observations on how the XML works.

but more importantly, it would...

b) create a very complex system of management for players since you would either have to memorize where and how many times you had used a multitude of rail units... or require a brand new method of tracking movement on rail (with an accompanying visual display) to be devised. While pondering this it also struck me that even with a graphical representation... this would become a very taxing exercise in micro management leading players to have to undo and redo moves during a turn trying to figure out how to make moves along this system work optimally.

That was purely the reason for trying to keep this idea simple.

Cernel

@hepps Right. I get that all you think you need it's already there, so it's up to @redrum, at the end. I still think that a count would be better than 1 to infinite only (tho I've to admit I've no plans for any right now), but this is mostly because I tend to dislike all the 1 to infinite options in general, for principle (starting from "givesMovement" and "isDestroyer"; there are so many options already that would be so much better if they only had a count I would make 1 to infinite illegal), but let's then assume the case of not having a count, in the sense of "rail" allowing up to a number of "train", instead.

I want to point out that in the particular case of trains and railroads (as long as you are not adding maybe depots in an AND relationship) it is totally realistic that a railroad allows moving infinite trains. In WW2 the limit was almost not railroads at all: it was the quantity of trains available.

Actually, rethinking about the @roiex proposal (of inverting AND and OR behaviourally from your current fomula), and letting aside all my additional suggestions, keeping as close as feasible to your formulation, as long as there is a count equal to 1 thereafter, I think the options in the same one may be logically understood in a OR relationship, as @roiex tends to see them anyway:

What the above would logically mean is that:

You need at least 1 of germanRail or russianRail (OR)
You need at least 1 of germanDepot or russianDepot (OR)
You need both the above (AND)

I believe that, logically and for consistency with most, if not all, the default count should be equal to the sum; thus if you have only:

Without any count, that would be an "AND" relationship (the assigned count would be equal to 2, in this case). You can assure the "OR" by setting the count equal to "1" and, moreover, this different formulation of mine would expand on what you can do, as you would be also able to set intermediate values; like if you have 4 units in the option, you can set the count also at 2 or 3, not only as 1 (OR) or 4 (AND).

I also believe that the "owned/allied" only, or whatever the like, restriction is redundant, and you should rather allow all units, comprising neutral (I mean the relationship) and enemy ones, to allow movement. Meaning you would be allowed to move into an enemy territory, if there are the required units (in the specific case, an enemy "rail" would allow "train" to move into, as long as the starting territory has a, likely owned, "rail" too), albeit enemy owned. This may make some sense for trains, depending on the scope of the map, as the enemy railways were captured and used to move your own trains to aid in your offensive, right in the days after it starts (so, you may have the trains as infrastructures giving an offensive support to other units; artillery comes to mind).
The above would be purely extensive (limiting it to owned and allied only is a redundant limitation), as the mapmakers that prefer trains (or whatever) to not jump on the enemy, in any case, can easily assure it by having, either:

Of the two above, the first one would factually enforce the same thing as your current (unnecessary) behaviour of excluding enemy units from the requirement. I normally suggest using it, for aaGuns and the like as well, instead of totally disabling combat movement, also as making more sense and being more user friendly.

Hepps

@cernel said in UnitAttachment Which Allows/Requires Another Unit To Move:

@hepps Right. I get that all you think you need it's already there, so it's up to @redrum, at the end.

I think you mistake my wish to keep the design simple (and quickly and easily integrated into the existing engine) with your desire to see broad changes to attachment functionality.

I'm not saying I disagree with what you are proposing... I'm saying I tried to make a proposal that did not involve changes on a massive scale that affect huge swaths of unit behavior.

redrum

@cernel @RoiEx Couple of points:

The format of AND/OR was taken from existing options such as "requiresUnits". So it would be a terrible idea to have the same format have different meaning for different options. I don't particularly think the format is great either way but would rather stick to be at least consistent:

requiresUnits values: is a list of units required to be present in the territory in order for you to build this unit there. Can have multiple instances. Only one instance needs to be true in order to build. examples: <option name="requiresUnits" value="CombatEngineer:Truck"/> would mean we need at least one CombatEngineer AND at least one Truck in order to build there. If after that line we had a second line saying <option name="requiresUnits" value="SuperEngineer"/> then it would mean OR we need at least one SuperEngineer.

Unless there are immediate use cases that map makers are looking to implement that need additional flexibility in this feature I'm not planning to add additional complexity in the off chance that someone would use it in the future.
I don't believe we have any features today that allow for different movement rates. It would most likely be a better approach to have territoryEffects/units increase/decrease movement costs for units rather than bonus movement. An example would be all territories cost 2 movement but if it has a road then it cost 1 movement. This is doable but definitely outside of this feature and much more complex do to its effect on path finding.

Cernel

@redrum I forgot about that.

Then, I definitely suggest refactoring the above so that, as per the example, instead of having:

<option name="requiresUnits" value="CombatEngineer:Truck"/>
<option name="requiresUnits" value="SuperEngineer"/>

you would have:
<option name="requiresUnits" value="SuperEngineer:CombatEngineer" count="1"/>
<option name="requiresUnits" value="SuperEngineer:Truck" count="1"/>

I think the maps having such occurrences should be very few, and I volunteer doing all the needed changes myself, if someone runs a mass search to give me the full list of maps having multiple "requiresUnits" on a same attachment, if that is possible (I doubt they are more than 5).

Notice that if you have only:
<option name="requiresUnits" value="CombatEngineer:Truck"/>

it would be exactly the same in both cases; so we don't have to worry about maps having a single "requiresUnits" per unit attachment.

Notice, in particular, the inconsistency with a closely related case:

consumesUnits requires that a unit be present in a territory, then when this unit is placed the other units are destroyed / upgraded into this unit. Can have multiple instances of this, which means it consumes multiple types of units.

<option name="consumesUnits" value="2:infantry"/>
<option name="consumesUnits" value="1:artillery"/>

In this case, it doesn't mean that you can either consume 2 infantries or 1 artillery; so here we really have a format inconsistency it would be better to refactor.

I took a look at Total World War right now and I see this:

    <attachment name="unitAttachment" attachTo="spanishFortification" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
      <option name="movement" value="0"/>
      <option name="attack" value="0"/>
      <option name="defense" value="10"/>
      <option name="hitPoints" value="2"/>
      <option name="isConstruction" value="true"/>
      <option name="constructionType" value="Fortr"/>
      <option name="constructionsPerTerrPerTypePerTurn" value="1"/>
      <option name="maxConstructionsPerTypePerTerr" value="3"/>
      <option name="requiresUnits" value="spanishEntrenchment"/>
      <option name="consumesUnits" value="1:spanishEntrenchment"/>
      <option name="consumesUnits" value="1:Material"/>
      <option name="requiresUnits" value="spanishCombatEngineer"/>
    </attachment>

I'm guessing this is a wrong or unintended coding in the map, because what this means is that you need 1 "spanishEntrenchment" OR 1 "spanishCombatEngineer".
Additionaly, since you have to consume "1:spanishEntrenchment" this implies that both the "requiresUnits" conditions are pointless, because the "spanishEntrenchment" is already mandated by the fact that you have to consume it, and, due to the OR relationship, the "spanishCombatEngineer" becomes unnecessary.

The above is present only for "spanishFortification", "swedishFortification", "turkishFortification", "brazilianFortification", that are all the neutral-ish players; thus I guess this is just a minor thing that has just never been fixed.

So, this would be a case of sorting this matter out and this, in my opinion, logically due refactor would help clarifying the matter here too.
As is, all the "requiresUnits" in that attachment could be removed, but I guess the intention was to require a "spanishCombatEngineer"; so I would remove only this line:
<option name="requiresUnits" value="spanishEntrenchment"/>
But notice that if you do my proposed changes, this would be unnecessary, as the two "requiresUnits" would then be in a AND relationship.
As I said, I can do the changes myself and push in in GitHub soon, but I guess @Hepps would prefer to deal with the matter himself, if he agrees with the refactor; up to him and no problem for me either ways.

Aside from the above, I see that the only thing that would need to be changed are these occurrences, in "Truck" and "Material".

      <option name="requiresUnits" value="germanFactory"/>
      <option name="requiresUnits" value="russianFactory"/>
      <option name="requiresUnits" value="japaneseFactory"/>
      <option name="requiresUnits" value="chineseFactory"/>
      <option name="requiresUnits" value="britishFactory"/>
      <option name="requiresUnits" value="italianFactory"/>
      <option name="requiresUnits" value="americanFactory"/>
      <option name="requiresUnits" value="spanishFactory"/>
      <option name="requiresUnits" value="swedishFactory"/>
      <option name="requiresUnits" value="turkishFactory"/>
      <option name="requiresUnits" value="brazilianFactory"/>

that I would change/push this way, if refactored:

  <option name="requiresUnits" value="germanFactory:russianFactory:japaneseFactory:chineseFactory:britishFactory:italianFactory:americanFactory:spanishFactory:swedishFactory:turkishFactory:brazilianFactory" count="1"/>

That would also make the attachment cleaner and more intuitive, because that whole bunch of "requiresUnits" doesn't look right, and it is really counterintuitive, as it logically means that you require all of them.

Plus this one, for "ResearchCenter":

  <option name="requiresUnits" value="germanCombatEngineer"/>
  <option name="requiresUnits" value="russianCombatEngineer"/>
  <option name="requiresUnits" value="britishCombatEngineer"/>
  <option name="requiresUnits" value="japaneseCombatEngineer"/>
  <option name="requiresUnits" value="italianCombatEngineer"/>
  <option name="requiresUnits" value="americanCombatEngineer"/>

that I'd change to:

  <option name="requiresUnits" value="germanCombatEngineer:russianCombatEngineer:britishCombatEngineer:japaneseCombatEngineer:italianCombatEngineer:americanCombatEngineer" count="1"/>

Again, I just need someone to run a search and give me the full list (surely Civil War will be one of the few) and I will do all the changes and pull requests in GitHub myself, if refactored, because this is really a thing that needs to be done.
It is an illogical and plainly wrong kind of coding, as @RoiEX correctly pointed out, and we should really seek to address it right now to avoid that this bad convention will going on plaguing more and more additional features in the future.
I'm sorry to have totally overlooked it.

I see that what is in the triggers in Total World War, meaning the various cases of:
<option name="unitProperty" value="requiresUnits" count="...

should be no problem as they are, in both cases.

I also took a look at Civil War. I can redo that in a few minutes. No problem.

So please let's do this refactor right now (I will do all the xml pushes if someone gives me the list of them) and let's avoid to keep expanding over new stuff what it is clearly an illogical system.

I can also do all the needed pushes before you refactor any (but you need to run the search and give me the list), then you refactor and merge all the pushes. Up to you. I really think this is due. Stuff should not be clearly illogical, especially if more to come the same wrong way.

Since it was not answered, in case it has been overlooked, I will repropose my suggestion of checking for presence of units no matter the relationship, thus comprising the case of enemy "rail" allowing movement of own "train" units.
There is really no reason to exclude hostile units here, because you can do it using <option name="attackingLimit" value="owned" count="0"/> on the unit attachment of the train or whatever (or alternatively also disable it from moving during combat move).
So, this would allow to have the additional ability of sending your trains to attack an enemy territory with rails in it, if you want to (as said, probably as infrastructures giving support). This would make the system more versatile for a number of other user cases beside "trains", at no loss at all.

redrum

@cernel Yeah, its kind of inconsistent between "requiresUnits" and "consumesUnits". The problem with refactoring is that it will break save game compatibility for any maps that use the parameters. So its better to wait til we have a non-backwards compatible release to make the changes.

Hepps

@cernel That is present for all fortifications.... for all nations.

Meaning that the existing entrenchment is both required and consumed to make a fortification. The material is then consumed for the up-grade and the Engineer is required to build the up-grade

redrum

@hepps The way it is specified in the XML actually means there needs to be either an entrenchment OR engineer (not AND). Since it consumes entrenchment as well, that is required no matter what so it should allow you to upgrade without an engineer present.

Hepps

@redrum Well that's not how the game works. You cannot build a fortification with a material and no engineer. Better people than me have tried.

redrum

@hepps Ha. Well I'm guessing if you try it then engine will allow you to upgrade without the engineer present (nobody probably ever tried since its clearly in the game rules).

I believe this would be the correct and most concise way for attachment (don't need entrenchment as required unit since it is a consumed unit):

<attachment name="unitAttachment" attachTo="spanishFortification" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
  <option name="movement" value="0"/>
  <option name="attack" value="0"/>
  <option name="defense" value="10"/>
  <option name="hitPoints" value="2"/>
  <option name="isConstruction" value="true"/>
  <option name="constructionType" value="Fortr"/>
  <option name="constructionsPerTerrPerTypePerTurn" value="1"/>
  <option name="maxConstructionsPerTypePerTerr" value="3"/>
  <option name="consumesUnits" value="1:spanishEntrenchment"/>
  <option name="consumesUnits" value="1:Material"/>
  <option name="requiresUnits" value="spanishCombatEngineer"/>
</attachment>

Hepps

@redrum Yes its been tried... and often people have to do edits to move the engineer they forgot to move in.

Cernel

@redrum Can you please run a search and give me the full list of games now (if it is an easy thing for you to do). Then I can do all the push requests that you can merge anytime thereafter.

I really think that the compatibility issues are a minor matter here. I'd get around doing this right away, I suggest. At most people will need to do a few edits and self-enforcing rules and you might have a couple bug reports to close; but probably not even that.

Once we have the full list, we can see what would be the matter with compatibility.

Doing the stuff in illogical ways is just a recipe for having confusions and bugs in the future, as your last discussion here I'm sure it's confirming to you.

As I said; that thing is only for "spanishFortification", "swedishFortification", "turkishFortification", "brazilianFortification", not for the big boys; so, it works fine for all the "normal" players; thus this is really a smaaaalllll matter, but it gives you the idea. I'll fix that too, np.

redrum

@cernel @Hepps Ah, yeah its only the neutrals that have the unitAttachment error, the rest do it the way I specified above. I don't have an easy way to get the list of maps as I don't have them all downloaded locally at the moment.

@cernel I think we should probably think about the best way to do AND and OR for options across the board before making any changes. I'm not that convinced that just inverting the 2 to be like consumesUnits is really that much better. I don't believe consumesUnits has anyway to do OR so you can only specify a single list of AND units that are required.

EDIT: Its probably best to make a new feature/refactor thread to continue this discussion as to not lose the discussion in this thread. For now this feature is essentially complete and a refactor would be done later on with an incompatible release anyways.

Hepps

@cernel Now I remember why this is written as it is for TWW

It is because the neutral nations were written in a very specific manner for when they we still neutral.

Cernel

@redrum No problem. You will give me the list when you can, then.

Do you want me to make all the push in advance? Please, confirm, if so.

I've seen that no regular games, comprising Global, would be influenced by this matter. So I think it would be only TWW and CW and maybe a couple others. I really think we can do this with very little worries about compatibility; but that's your call, of course.

I think we should probably think about the best way to do AND and OR for options across the board before making any changes. I'm not that convinced that just inverting the 2 to be like consumesUnits is really that much better.

Inverting was what @RoiEX said. My proposal is actually not just inverting, but needing the additional "count" equal to 1 for having the "OR". This also means, as I said, that if you have only 1 occurrence of "requiresUnits" in the attachment, then nothing changes (it is AND in old and new alike), which limits the compatibility problems really to like nothing.

Cernel

@redrum said in UnitAttachment Which Allows/Requires Another Unit To Move:

I don't believe consumesUnits has anyway to do OR so you can only specify a single list of AND units that are required.

Yeah, it would be really good if you can have a "OR" for consuming units (for example, upgrading a "paratrooper" from "infantry" or "elite"); but that would also require a selection window.

Cernel

@hepps I saw that, due to the purchase limits of the old Fodder AI", with the triggers bringing back the stuff, when it is going to be assigned to the players. But this is another matter yet. As I said, the two "requiresUnits" here are just useless. You could remove them both, and nothing would change, for those units. As I said, this is really a marginal matter, we/you can solve any ways you prefer (most likely, what is wanted is removing the first occurrence only), doesn't really matter and it's no problem; it was really not the focus of my post.
As a matter of my proposal, if you want just to keep it as it is, the change would be this one (but this would equal not having it):
<option name="requiresUnits" value="spanishEntrenchment:spanishCombatEngineer" count="1"/>