Opened 21 months ago

Closed 20 months ago

Last modified 17 months ago

#3792 closed defect (fixed)

[PATCH] Stackable auras

Reported by: fatherbushido Owned by: sanderd17
Priority: Must Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by fatherbushido)

Actually, auras are not stackable. But there is some side effects with capture. For example : If one have two macedonians theater (one and one captured), auras don't stack. But if one have one macedonian theater and one spartan theater (captured), auras stack.

Same with temple for example.

Attachments (19)

3792.diff (3.7 KB) - added by fatherbushido 21 months ago.
3792.2.diff (1.3 KB) - added by fatherbushido 21 months ago.
rebased on 17810
stackablewonder.diff (740 bytes) - added by fatherbushido 21 months ago.
This sets wonder as stackable.
3792.3.diff (1.3 KB) - added by fatherbushido 21 months ago.
3792.4.diff (1.3 KB) - added by fatherbushido 21 months ago.
auras.7z (4.1 KB) - added by fatherbushido 21 months ago.
json files folder to put in simulation/data/
patch.diff (83.1 KB) - added by fatherbushido 21 months ago.
test it
auras_stable.7z (4.1 KB) - added by fatherbushido 21 months ago.
patch_stable.diff (84.0 KB) - added by fatherbushido 21 months ago.
patch_stable.2.diff (86.1 KB) - added by fatherbushido 21 months ago.
aura_patch.diff (83.6 KB) - added by fatherbushido 20 months ago.
aura_data.7z (3.2 KB) - added by fatherbushido 20 months ago.
aura_patch.2.diff (83.6 KB) - added by fatherbushido 20 months ago.
aura_patch.3.diff (84.1 KB) - added by fatherbushido 20 months ago.
aura_patch.4.diff (84.5 KB) - added by fatherbushido 20 months ago.
auras.2.7z (3.2 KB) - added by fatherbushido 20 months ago.
aura_patch.5.diff (84.5 KB) - added by fatherbushido 20 months ago.
stackable.diff (1.9 KB) - added by fatherbushido 20 months ago.
stackable flag for last patch, wonder stackable as example
stackable.2.diff (2.7 KB) - added by fatherbushido 20 months ago.

Download all attachments as: .zip

Change History (41)

comment:1 Changed 21 months ago by fatherbushido

As discussed on irc with elexis and sanderd17, we can use other robust keys/identifiers for auras such that auras never stack. We also add an optional flag in auras template which always allow stacking.

comment:2 Changed 21 months ago by fatherbushido

Summary: Stackable auras[PATCH] Stackable auras

Changed 21 months ago by fatherbushido

Attachment: 3792.diff added

Changed 21 months ago by fatherbushido

Attachment: 3792.2.diff added

rebased on 17810

Changed 21 months ago by fatherbushido

Attachment: stackablewonder.diff added

This sets wonder as stackable.

comment:3 Changed 21 months ago by fatherbushido

According to our discussion :/irclogs, there is a problematic case. If player with civ A have for example a temple with an aura wich modify the same value by adding 5 and a player with civ B have a temple with an aura wich modify the same value by adding 10, what is expected ?

The patch have a good reaction and affects units with only one of this aura (the first which applies). But this situation may not occurs since it doesn't make sense. One solution that mimo's give is the following :

  • it would be nice to ensure that all auras have the same value for all civs
  • differences would be introduced by civ specific technologies (now that they can apply to auras) possibly autoresearched

So as tech will apply to the player civ, we don't care if it's temple A or B which applies its aura, since the value will be the same.

comment:4 Changed 21 months ago by fatherbushido

Description: modified (diff)

Changed 21 months ago by fatherbushido

Attachment: 3792.3.diff added

Changed 21 months ago by fatherbushido

Attachment: 3792.4.diff added

comment:5 Changed 21 months ago by fatherbushido

Sum up.

  • what is an Aura ?

An aura is a set of modifications. An aura is a component of an entity and targets some entities.

  • Can a target unit be affected by 2 same auras at the same time ?

The postulate is : no.

  • How can we check this ?

There is now many auras and the set of their modifications is not one to one associated with their names. So most of the auras have the same name (typically Aura1, Aura2, or sometimes Heal). (I think at first design an aura with a defined name was expected to do a defined set of modifications). So we must define an identifier (a key) that is in one to one correspondence with the set of modifications of the aura. A natural choice is : (name + "/" + mod.value + "/" + (mod.add || 0) + "/" + (mod.multiply || 1)) as it contains the set of modifications !

And if i wan't that an Aura can be applied twice ?

Here we add a stackable flag and use an unique key for each source of the Aura (name + "/" + mod.value + "/" + this.entity)

  • So what's the without patch and with the patch problem ?
  • situation 1

A temple T1 of civ C1 have an aura who add 1 to Health.Max A temple T2 of civ C2 have an aura who add 1 to Health.Max

  • current situation :

If player 1 have temple T1 and capture temple T2, units are affected by both auras (they stack) If player 1 have temple T1 and build antoher temple T1, units are affected by one aura (they don't stack)

  • with patch :

auras don't stack (identifier is the same) if we add stackable flag, they stack.

  • situation 2

A temple T1 of civ C1 have an aura who add 1 to Health.Max A barrack B1 of civ C1 have an aura who add 2 to Health.Max

  • current situation

Auras stacks (template name of the source is part of the actual key)

  • with patch

Auras stacks (modifications are different, so the identifiers / the auras are different).

  • situation 3

A temple T1 of civ C1 have an aura who add 1 to Health.Max A barrack B1 of civ C1 have an aura who add 1 to Health.Max

  • current situation :

If player 1 build temple T1 and barrack B1, auras stack.(template name of the source is part of the actual key) It is perhaps what we can expected but it doesn't match the postulate. Here the auras are the same, so they shouldn't stack.

  • with patch :

auras don't stack (identifier/auras are the same). But in this particular case, we can expect that the man who writes the templates wants that they stack. In this case, he needs to add the stackable flag.

  • situation 4

A temple T1 of civ C1 have an aura who add 1 to Health.Max A temple T2 of civ C2 have an aura who add 2 to Health.Max

  • current situation

Auras stack (because the civ are differents and is in the actual identifier of auras)

  • with patch

Auras stack (because the modifications / auras are differents).

Perhaps it's not what the template writer wants. But how can we expect what he wants in this case ? For this case (using specific civ modifications of an aura), the template writer can use an autoresearch tech which affects this aura (meta modifications of auras).

  • sum up

The patch has the expected behavior in each case. It fixs the bug and is ok with the postulate : "a same aura don't apply twice" (an aura is a set of modifications of a definite value). If we don't want that, we use the stackable flag. So template's writer can have what he wants.

Last edited 21 months ago by fatherbushido (previous) (diff)

comment:6 Changed 21 months ago by elexis

Keywords: review removed

After a long discussion with leper and sanderd17, we decided that the only solution can be to move all auras from XML to JSON files again.

This way we can use the filename as a unique identifier instead of relying on workarounds and weird assumptions and running into edgecases.

comment:7 Changed 21 months ago by fatherbushido

I m working on it.

comment:8 Changed 21 months ago by Itms

Milestone: Alpha 20Alpha 21

This will change a lot of things and it would be dangerous to include it in A20.

comment:9 Changed 21 months ago by fatherbushido

I moved back xml to json. I modified Auras.js code and reimplement meta modifications of auras by technologies. Actually i have some issue with tooltips and structure tree. Both call GetTemplateDataHelper @ globalscripts/Template.js. As auraDescription and auraName aren't in template anymore, i call this info in the JSON file. It needs to be optimized. But, this globalscripts is used

  • by gui/common/tooltips.js
  • by gui/structure/tree/helper.js and load.js

which read the file in different path (structure tree forget simulation/data).

see :irc for ideas.

Last edited 21 months ago by fatherbushido (previous) (diff)

comment:10 Changed 21 months ago by sanderd17

You need watch out, the Engine.ReadJSONFile will return null when the JSON file doesn't exist. It would be best to show at least a warning mentionning the filename instead of just having a null-reference error.

Perhaps the Engine call can be moved into a separate function of the global Templates.js, so it can be called from the simulation and have the same error handling (and later on, the global helper could hold some kind of sensible cache if it's needed, the cache doesn't have to be serialised anyway).

You also need to add the attribute=tokens to the XML, else f.e. the garrison aura of the general hero template won't be inherited by other heroes.

The rest is quite ok, but still has to be tested

For the sake of the diff, it would probably be best to include your json files in a separate zip. Else, working with a whole bunch of new files can become a bit annoying when testing the patch.

comment:11 Changed 21 months ago by fatherbushido

Thanks for the comments, the hints and the advices.

I will adress them.

Last edited 21 months ago by fatherbushido (previous) (diff)

Changed 21 months ago by fatherbushido

Attachment: auras.7z added

json files folder to put in simulation/data/

Changed 21 months ago by fatherbushido

Attachment: patch.diff added

test it

Changed 21 months ago by fatherbushido

Attachment: auras_stable.7z added

comment:12 Changed 21 months ago by fatherbushido

Ref : #3830 #3522 #3792.

  • moving all to json
  • adding requiredTechnology for auras (following first idea of Mimo and improvement of Sanderd17, we can modify an aura by technology using two auras, one of them has a requiredTechnology).
  • it lacks only the wall aura tooltip in structure tree
Last edited 20 months ago by fatherbushido (previous) (diff)

Changed 21 months ago by fatherbushido

Attachment: patch_stable.diff added

Changed 21 months ago by fatherbushido

Attachment: patch_stable.2.diff added

Changed 20 months ago by fatherbushido

Attachment: aura_patch.diff added

Changed 20 months ago by fatherbushido

Attachment: aura_data.7z added

comment:13 Changed 20 months ago by fatherbushido

Just look at aura_patch.diff​ (and aura_data.7z). Previous patches are bullcrap.

comment:14 Changed 20 months ago by fatherbushido

Description: modified (diff)

Changed 20 months ago by fatherbushido

Attachment: aura_patch.2.diff added

comment:15 Changed 20 months ago by fatherbushido

Keywords: review added

comment:16 Changed 20 months ago by sanderd17

A code review of the latest patch.

The biggest problem is that the tech-enabling of auras doesn't work. The cleaning uses the affected units to re-apply the auras to them. But auras that aren't enabled don't affect any units. So the cleaning has no effect (unless it's a global or range aura). This part will have to be redone, but it can be ripped out for now (for simplicity of the first commit).

Next to that, some small remarks:

  • The "aurasTemplate" parameter needs to be documented in the javaDoc above the function (if there's JavaDoc?, it should be correct).
  • cmpTechnologyTemplateManager should be renamed now, though that's not for this patch (renaming files gets annoying with SVN)
  • Lines 597-600 in GuiInterface?.js don't have a null-check and a double if with the same arguments.

The rest of the patch looks fine.

Changed 20 months ago by fatherbushido

Attachment: aura_patch.3.diff added

Changed 20 months ago by fatherbushido

Attachment: aura_patch.4.diff added

Changed 20 months ago by fatherbushido

Attachment: auras.2.7z added

Changed 20 months ago by fatherbushido

Attachment: aura_patch.5.diff added

comment:17 Changed 20 months ago by sanderd17

In 17974:

Cleanup of the Auras code in preparation of stackable auras. Implements auras being enabled by techs. Refs #3792. Patch by fatherbushido

Changed 20 months ago by fatherbushido

Attachment: stackable.diff added

stackable flag for last patch, wonder stackable as example

Changed 20 months ago by fatherbushido

Attachment: stackable.2.diff added

comment:18 Changed 20 months ago by sanderd17

Owner: set to sanderd17
Resolution: fixed
Status: newclosed

In 17982:

Implement stackable auras + fix an issue with techs, auras and ownership changes. Patch by fatherbushido. Fixes #3792

comment:19 Changed 20 months ago by sanderd17

Keywords: review removed

comment:20 Changed 19 months ago by elexis

In 18076:

Revert a copy&paste mistake in r17974 when auras were converted from XML to JSON. Patch by fatherbushido, refs #3792.

comment:21 Changed 19 months ago by elexis

In 18188:

Fix typo in r17974. Patch by fatherbushido, refs #3792.

comment:22 Changed 17 months ago by elexis

In 18400:

Fix indentation of auras in r17974. Patch by fatherbushido, reported by leper, fixes #4062, refs #3792.

Note: See TracTickets for help on using tickets.