[PATCH] Trade can occur between two incomplete buildings

[gudo]

When setting up a trade route, it is possible to trade between two incomplete structures.

Place two Market foundations, but do not build them. Attempt to establish a trade route between the two Trade will happen successfully.

[historic_bruno]

At first glance it seems like a good solution for the UI side of things. Usually we add checks to both UI and simulation (so that modified UIs, AIs, etc., can't accidentally or intentionally trigger these bugs). Is there a corresponding check we can perform for the simulation? Relevant code would probably be Trader.CanTrade in Trader.js.

[O.Davoodi]

Attached new patches for ticket 1215. Added the check to simulation. Note:Ignore that "train" part in input.js please.

[O.Davoodi]

attached a newer patch. removed the lines from other tickets. but i'll commit them together.

[historic_bruno]

Replying to Spahbod:

attached a newer patch. removed the lines from other tickets. but i'll commit them together.

How are you creating the patches? Could you apply them all locally and then create a single diff/patch from the root directory of your working copy? That makes it a bit easier to review and test :)

[O.Davoodi]

For reasons unknown I can't create a single patch. It seems that my session, helpers and components svn folders are separate. I can't make a single patch of them.

[leper]

Do you really need to pass targetState? You can achieve the same thing with using the already present cmpTargetIdentity to check if the target is a foundation. Maybe you need to create a cmpTargetFoundation to check this.

But code like if (!this.CanTrade(currentMarket, currentMarket)) seems strange.

[O.Davoodi]

Replying to leper:

Do you really need to pass targetState? You can achieve the same thing with using the already present cmpTargetIdentity to check if the target is a foundation. Maybe you need to create a cmpTargetFoundation to check this.

But code like if (!this.CanTrade(currentMarket, currentMarket)) seems strange.

I tried cmpTargetIdentity and got a bunch of errors and warnings.

But I changed the patch.

[O.Davoodi]

Much a cleaner way.

[leper]

Sorry, I meant using Foundation.js as you do now.

You have some changes from your batch training patch in finalPatch2.patch. I would also name the variable like the others. Like cmpTargetFoundation instead of targetState. Apart from that the patch is looking good, though I am not really able to review it :-) .

[historic_bruno]

Replying to leper:

I would also name the variable like the others. Like cmpTargetFoundation instead of targetState. Apart from that the patch is looking good, though I am not really able to review it :-) .

Indeed the convention is to name component references like "cmpFoundation", although I don't think it's been added to Coding_Conventions yet. (The reason we use e.g. targetState in GUI scripts is because they don't access components directly, only a limited entity state created by GUIInterface so it would be wrong to call them cmpFoo.)

I'll move the SVN conversation to forum messages to avoid cluttering this ticket.

[Philip Taylor]

Yep, it should be cmpTargetFoundation to be consistent with the other variables in that function.

Couple of things in input.js:

• Needs spaces around the "&&"
• Rather than indenting the switch and adding a long if/else, better to do

  if (tradingDetails === null)
     return {"possible": false};
  switch (...)
  {
      ...
  }
  

  to minimise the distance between the error condition and associated return.

[O.Davoodi]

Patch that fixes the problem of trading with incomplete markets.

[O.Davoodi]

Updated the patch.

[leper]

In 11440:

Prevent trade with foundations, based on patch by Spahbod, fixes #1215.

[leper]

Thanks for the patch. I applied a slightly modified version stripped of the code from other patches.