Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add apply_to=set_variable #4528

Open
gfgtdf opened this issue Oct 30, 2019 · 11 comments
Labels

Comments

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented Oct 30, 2019

I think it'd be nice to have a set_variable [effect] that sets a unit variable, in order to make removing if these object work the unit variable that is changed should be a child of the [mods] node because that node is cleared by the engine when the unit is reset.

variables_.clear_children("mods");

So my current question is should a set_varible [effect] only allow setting a variable inside the [mods] more or should.it allow changing any variable? While allowing the not mods makes sense, I somehow think.its easier to understand if a code like

[effect]
apply_to=set_variable
name=my_abiltit_strengh
add=5
[/Effect]

would increase $unit.variables.my_abiltit_strengh instead of $unit.variables.mods.my_abiltit_strengh

but of course that mean that people had to write

[effect]
apply_to=set_variable
name=mods.my_abiltit_strengh
add=5
[/Effect]
@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

@ProditorMagnus ProditorMagnus commented Oct 30, 2019

I dont have use case for setting unit variables from [effect].

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Oct 30, 2019

I dont have use case for setting unit variables from [effect].

Hmm not sure whether you are asking me for a usecase or whether you just wanted to say that you have no opinion on this issue.

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

@ProditorMagnus ProditorMagnus commented Oct 30, 2019

I think creating such save-points for batches of variables updates would be quite complex. How would it work if unit has variable, effect sets it to another value, then there is manual update to third value, and then effect is removed? Since setting of third value could depend on second value.

So I ask for reasoning for saving history of variable value changes. Instead of just setting the variable.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Oct 30, 2019

I think creating such save-points for batches of variables updates would be quite complex, How would it work if unit has variable, effect sets it to another value, then there is manual update to third value, and then effect is removed?

The manual.update is discarded (assuming you are changing the mods subnode), just like it happens for other unit attributes when an object is deleted.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 1, 2019

Personally, instead of adding an apply_to=set_variable, I think we should encourage people to define custom effects for the specific variables they need to work with.

In particular, the effect system seems to work counter to the idea of unit variables – effects are reapplied on level-up, but variables are persistent.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 1, 2019

In particular, the effect system seems to work counter to the idea of unit variables – effects are reapplied on level-up, but variables are persistent.

That'd what I said vegorey: except the [mods] subtag of a unit varaibles which is no persistent for exactly this reason.( To be usable in effects)

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 2, 2019

I think that's still strange though. Why should any one subtag of [variables] be treated differently? On the whole I think it's probably a bad idea to mix effects and variables. Perhaps a better way would be to include the variable data directly in the [object] tag? It can still be tested like a variable (though it takes a little more effort), but it'll be properly reset when the unit is rebuilt without that object. The "little more effort" could be implemented as helper methods in Lua so that add-on developers don't need to reinvent the wheel (it would be stuff like unit.get_object(id), unit.get_object(tag), and anything else that seems useful).

The other possibility would be to add an additional method that's read from a custom effect's metatable, which is supposed to reverse the effect. Then as long as people manipulate objects solely through add/remove modification, it should be possible to keep it all consistent.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 2, 2019

I think that's still strange though. Why should any one subtag of [variables] be treated differently? On the whole I think it's probably a bad idea to mix effects and variables.

To make exactly this work: someones you want variables that are persitent over unit rebuilds and sometimes you want variables that are not (in face we have the exact same problem with statuses, and there afaik are/were know bugs causes by stuff like unpoisonable no removed after unit advance). We could of course also have a lua on_unit_rebuild hook where the user could clear the variable himself. But i think making this in the engine has not only the advantage that its trivial to implement (the current implementation is a oneliner), but its also now easy to see directly form the variablename (beginning with mods. or not) whether they are cleared on reset or not.

Perhaps a better way would be to include the variable data directly in the [object] tag? It can still be tested like a variable (though it takes a little more effort), but it'll be properly reset when the unit is rebuilt without that object. The "little more effort" could be implemented as helper methods in Lua so that add-on developers don't need to reinvent the wheel (it would be stuff like unit.get_object(id), unit.get_object(tag), and anything else that seems useful).

well the usecase it usually not only to 'set' a variable but also to increase it etc. Let me give you the case where i currenlty use it in one of my 1.12 addons: here it is set

https://github.com/gfgtdf/Scenario_with_robots/blob/cd8d800e57b59317a9d08400af6bd5737f3a17a9/lua/effects.lua#L78

and here it is read:

https://github.com/gfgtdf/Scenario_with_robots/blob/cd8d800e57b59317a9d08400af6bd5737f3a17a9/lua/wml_codes.lua#L203
(the second one generates a ability wml code from lua)
(okay why does it not show the code inline and just show the links?)

The other possibility would be to add an additional method that's read from a custom effect's metatable, which is supposed to reverse the effect. Then as long as people manipulate objects solely through add/remove modification, it should be possible to keep it all consistent.

No asume you first apply a "x2" effect and then a "+2" effect and then remove the "x2" effect, if the implementation of reversing the x2 effect would be division by 2 then the end value woudl be one less then it should.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 3, 2019

But i think making this in the engine has not only the advantage that its trivial to implement (the current implementation is a oneliner)

The implementation of apply_to=variable is only sane if you accept that the variable is cleared before any unit rebuild, but that's not the case in a general sense.

well the usecase it usually not only to 'set' a variable but also to increase it

Yes, point taken, I guess my suggestion isn't as good as it first seemed... but I still think the addition of apply_to=variable also isn't good with the current system.

I guess what you want is to be able to add custom "stats" or "attributes" to units, which can be manipulated through effects in the same way as built-in attributes like hit points or movement.

If key-value attributes are sufficient, you could simply add a new [attributes] tag that parallels the [status] tag but takes string values instead of boolean values. That allows the feature you need without polluting the unit variables namespace with special magic variables that are treated differently. It would also allow you to support specifying defaults for the attributes in the [unit_type].

If key-value data isn't sufficient, this idea could easily be expanded into supporting generic WML content as attributes, though needing to go that far would make me question the idea in the first place.

On the other hand, if you're dead set on allowing apply_to=variable in the first place, then the entire process of applying modifications and rebuilding the unit needs to be changed. There would need to be a "reverse" hook in the effect definition metatable like I said earlier, and when rebuilding a unit, the reverse hook for every effect would have to be run in reverse order. Built-in effects would have a no-op reverse hook, because they affect things that are reset by the process already. The variable effect would need to either invert the operation that it applied to the variable (which is a problem because not all operations are invertible) or somehow store the variable's old value so that it can be restored… which is also tricky, because there's no obvious persistent storage accessible to the effect's function (perhaps it could be given access to the modification's WML, I suppose, but even that would be problematic - what if you have multiple variable effects in the same modification?).

I would definitely prefer something closer to the first idea (an [attributes] tag), but I can't be sure whether that satisfies your use case.

(Oh, by the way, I noticed that you have bad translatable strings in your second link. A rule of thumb is: don't concatenate translatable strings with anything, ever. You can make an exception only if each string is a full sentence.)

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 3, 2019

The implementation of apply_to=variable is only sane if you accept that the variable is cleared before any unit rebuild, but that's not the case in a general sense.

Yes that's why i have the main question of my first post: should we restrict apply_to_varibale to only allow changing the mods subtable. also ntoe that not all effects that we currently have are 'sane', for example "experience", "movement" and certain statuses.

I guess what you want is to be able to add custom "stats" or "attributes" to units, which can be manipulated through effects in the same way as built-in attributes like hit points or movement.

Yes this is the main usecase currently.

If key-value attributes are sufficient, you could simply add a new [attributes] tag that parallels the [status]

Well is we do this already we can have aswell new tag that parrales the [variables]

That allows the feature you need without polluting the unit variables namespace with special magic variables that are treated differently

My point is that 1) i don't see much of a problem with mods being treated differently, 2) this is already 1.12 behaviour. (my linked code works is the 1.12 version of my addon)

There would need to be a "reverse" hook in the effect definition metatable

as i said on my previous post, this is not really possible,

(Oh, by the way, I noticed that you have bad translatable strings in your second link. A rule of thumb is: don't concatenate translatable strings with anything, ever. You can make an exception only if each string is a full sentence.)

Yes i know, i wrote that code a long time ago and didn't bother to update it yet.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 3, 2019

also ntoe that not all effects that we currently have are 'sane', for example "experience", "movement" and certain statuses.

Yeah, that's true, you have a point there.

Well is we do this already we can have aswell new tag that parrales the [variables]

No, because it wouldn't accept subtags of any kind, only keys.

as i said on my previous post, this is not really possible,

It's not possible only because of the current implementation of how modifications are removed and units are rebuilt. But that implementation can be changed, as I described in my previous post. Did you even actually read the paragraph you're quoting here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.