#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 )
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)
Change History (41)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Summary: | Stackable auras → [PATCH] Stackable auras |
---|
by , 9 years ago
comment:3 by , 9 years ago
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 by , 9 years ago
Description: | modified (diff) |
---|
by , 9 years ago
Attachment: | 3792.3.diff added |
---|
by , 9 years ago
Attachment: | 3792.4.diff added |
---|
comment:5 by , 9 years ago
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.
comment:6 by , 9 years ago
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:8 by , 9 years ago
Milestone: | Alpha 20 → Alpha 21 |
---|
This will change a lot of things and it would be dangerous to include it in A20.
comment:9 by , 9 years ago
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.
comment:10 by , 9 years ago
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 by , 9 years ago
Thanks for the comments.
- I have put the json files in an attached 7z. To put in simulation/data/auras.
- I modified all xml files and auras schema to use tokens.
Actually no problem with downloading all units in scenario demo unit map.
We still need to fix structure tree (which imo is not so related to the patch himself ; i need to study that to understand it more deeply) and to improve code following the hints you give above.
- I have a bug. I must check if it's my fault or something else. When building a wonder, i have the +10 pop bonus ; then i capture my ennemy wonder, as we don't set things stackable for the moment i have still the +10 pop bonus. Then destroying one of the wonder, i lost both bonus.
by , 9 years ago
Attachment: | auras_stable.7z added |
---|
comment:12 by , 9 years ago
- 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
by , 9 years ago
Attachment: | patch_stable.diff added |
---|
by , 9 years ago
Attachment: | patch_stable.2.diff added |
---|
by , 9 years ago
Attachment: | aura_patch.diff added |
---|
by , 9 years ago
Attachment: | aura_data.7z added |
---|
comment:13 by , 9 years ago
Just look at aura_patch.diff (and aura_data.7z). Previous patches are bullcrap.
comment:14 by , 9 years ago
Description: | modified (diff) |
---|
by , 8 years ago
Attachment: | aura_patch.2.diff added |
---|
comment:15 by , 8 years ago
Keywords: | review added |
---|
comment:16 by , 8 years ago
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.
by , 8 years ago
Attachment: | aura_patch.3.diff added |
---|
by , 8 years ago
Attachment: | aura_patch.4.diff added |
---|
by , 8 years ago
Attachment: | auras.2.7z added |
---|
by , 8 years ago
Attachment: | aura_patch.5.diff added |
---|
by , 8 years ago
Attachment: | stackable.diff added |
---|
stackable flag for last patch, wonder stackable as example
by , 8 years ago
Attachment: | stackable.2.diff added |
---|
comment:19 by , 8 years ago
Keywords: | review removed |
---|
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.