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

remove code for removing apply_to=type [effect]s #4542

Open
gfgtdf opened this issue Nov 3, 2019 · 16 comments
Labels

Comments

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented Nov 3, 2019

At 1.12 there was a code tried to fix [remove_object], when using it to remove a apply_to=type effect, by storing the previous type in the effect and applying that when the object is remvoed. However this code was broken in 1.13 by

0f1b21a

(clearly setting the local variable std::string prev_type has no effect), and also so far noone cried about it, while it would of course be trivial to restore the 1.12 behviour in 1.15, think its better to remove this code becasue the implementation really only works in the most basic case:

  1. It probably does not work poperly when the unit advanced again after the object was applied.
  2. It does not work for effects that change variation
  3. It's also unclear how ts suposed to work for [effect] with apply_to=type that have a filter, that is when the filter is no longer true, the typechanging effect is not reverted when te unit is rebuild (unliek other effect)
@gfgtdf gfgtdf added the WML label Nov 3, 2019
@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 3, 2019

also from looking at the code it seems like apply_to=variation/type does not work in [trait]s at all.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 3, 2019

I feel like these two effects are kinda contrary to every other effect, because they change the baseline that the effects are built off of. I think it would be better to deprecate them and recommend people use [transform_unit] or whatever instead, but that would break anything that uses them in [trait] or [advancement]… maybe add type/variation keys in [advancement] for this purpose? I dunno…

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 3, 2019

the effects are built off of. I think it would be better to deprecate them and recommend people use [transform_unit] or whatever instead, but that would break anything that uses them in [trait] or [advancement]… maybe add type/variation keys in [advancement] for this purpose? I dunno…

yes i had the same idea, but probably no worth the compatability by break, not sure though.

@Wedge009 Wedge009 added the Enhancement label Nov 3, 2019
@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 4, 2019

Well, perhaps we should consider a few questions:

  1. Is there any reason whatsoever to use apply_to=type or apply_to=variation in a [trait]? Do any add-ons already do this, either in 1.12 or 1.14?
  2. Is there any reason to use apply_to=type in an [advancement]? My instinct would be to say no – just add the type to advances_to instead – but maybe I'm missing something. Do any add-ons do this?
  3. Is there any reason to use apply_to=variation in an [advancement]? My instinct would be to say yes, in which case a new method might need to be added. Perhaps an advances_to_variation key to parallel advances_to?
  4. Is there any reason whatsoever to use apply_to=type or apply_to=variation in an [object] that couldn't already be handled by a different means? My instinct is no. If it's ActionWML, you can use [transform_unit] for the same effect. If it's in a unit definition, for example in SideWML, then you can just set the target type and variation directly instead of through an object. Do any add-ons do this in ways that can't be substituted with one of the aforementioned methods?

The main thing to pay attention to would be cases that mix type/variation effects with other effects. Would using a different method still allow those to work in the same way?

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 4, 2019

Well, perhaps we should consider a few questions:

  1. Is there any reason whatsoever to use apply_to=type or apply_to=variation in a [trait]? Do any add-ons already do this, either in 1.12 or 1.14?

Well probaly not as I said in my second comment, I think these effects didn't work on traits in the first place ( unless the trait is added manually)

  1. Is there any reason to use apply_to=type in an [advancement]? My instinct would be to say no – just add the type to advances_to instead – but maybe I'm missing something. Do any add-ons do this?

One could for example want an advanement that changes the type and also does something else.

  1. Is there any reason to use apply_to=variation in an [advancement]? My instinct would be to say yes, in which case a new method might need to be added. Perhaps an advances_to_variation key to parallel advances_to?

Yes. the mainline campaigns advancement wml code, see for example the recently merged pr about DiD, uses variations in advancements, also combined with other effects.

  1. Is there any reason whatsoever to use apply_to=type or apply_to=variation in an [object] that couldn't already be handled by a different means? My instinct is no. If it's ActionWML, you can use [transform_unit] for the same effect. If it's in a unit definition, for example in SideWML, then you can just set the target type and variation directly instead of through an object. Do any add-ons do this in ways that can't be substituted with one of the aforementioned methods?

I don't think there.is a good usecase for these effects in [object]

The main thing to pay attention to would be cases that mix type/variation effects with other effects. Would using a different method still allow those to work in the same way?

depends what the different method is of cpurse if we add for example add new_type/variation to [advancement] it would.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 4, 2019

Should definitely check with add-ons, but from your response it does seem like [advancement] is the only place where these effects are useful.

Well probaly not as I said in my second comment, I think these effects didn't work on traits in the first place ( unless the trait is added manually)

Is this the case in 1.12 as well, I wonder?

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 4, 2019

Okay I think we both agree that the type/variation [effect]s should not have been added to the game.in their current way. The question now.is whtger it's worth doing the comparability break/deprecation over this.

Is this the case in 1.12 as well, I wonder?

I think so but I did not test.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor

@newfrenchy83 newfrenchy83 commented Nov 5, 2019

Effect apply_to=type is already used in utbs for in youth.cfg advancement tree for Kaleh and Nym characters

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 5, 2019

I think so but I did not test.

Probably should, since this area did change between 1.12 and 1.14.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 5, 2019

I looked at the code https://github.com/wesnoth/wesnoth/blob/1.12/src/unit.cpp#L729 it's basicially the same the traits just get added to the trait list, but the type/variation effect are appt effective when added with add_modifiarion noadd=false

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 7, 2019

I noticed your wiki edit, which implies it does work properly in [advancement], did you check that?

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

@gfgtdf gfgtdf commented Nov 7, 2019

Newfrencjy just said it's used that way in mainline

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 7, 2019

Ah, true, but that's for type, not variation, if I'm not mistaken? I seem to recall @nemaara saying variation doesn't work.

@nemaara

This comment has been minimized.

Copy link
Contributor

@nemaara nemaara commented Nov 7, 2019

Err, apply_to=variation works, but you can't use apply_to=type followed by apply_to=variation (it doesn't take you to a variation of the new type). Also, you can't use variation= in apply_to=type.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 8, 2019

Ah, okay.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Nov 10, 2019

Related: The bug for changing both type and variation at the same time should be fixed in 217992a

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