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

WIP Fix remove overlay #4088

Open
wants to merge 14 commits into
base: master
from

Conversation

@gfgtdf
Copy link
Contributor

commented May 20, 2019

fixes remove_unit_overlay after unit_overlay change after 1a7724e#diff-d471df014b92a45ed106cf664118b43c

fixes LOYAL overlay disappearing from units after levelup.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

i had think to use this method of insert this in traits but i tried other method before.

@@ -164,7 +164,7 @@ _"No gold carried over to the next scenario."#enddef
{TRAIT_UNDEAD}
{TRAIT_LOYAL}
[/modifications]
overlays="misc/loyal-icon.png"
{IS_LOYAL}

This comment has been minimized.

Copy link
@stevecotton

stevecotton Aug 8, 2019

Contributor

Does IS_LOYAL do anything useful any more?

If the unit already has TRAIT_LOYAL then that will add the overlay, and if it doesn't then IS_LOYAL will be a bug-in-waiting with the overlay disappearing when the unit levels up.

This comment has been minimized.

Copy link
@gfgtdf

gfgtdf Aug 10, 2019

Author Contributor

in the current state of the pr it still adds the ´overlays="misc/loyal-icon.png"´ line, this is wip though, so im not 100% sure how i want ti to be in the end. This specific commit was mostly made before the actual change as a cleanup to make working on it easier.

@@ -550,7 +550,7 @@
name=EthilielV.modifications
mode=insert
[value]
{TRAIT_LOYAL}
{TRAIT_LOYAL OVERLAY=""}

This comment has been minimized.

Copy link
@stevecotton

stevecotton Aug 8, 2019

Contributor

I can't justify it, but my gut feeling is that TRAIT_LOYAL_NO_OVERLAY feels better than a macro with an argument.

This comment has been minimized.

Copy link
@gfgtdf

gfgtdf Aug 10, 2019

Author Contributor

we could, i don't have a strong opinion on it.

@stevecotton

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

I think it would make sense to deprecate support for [unit]overlay=, and then verify with the Schema validator that it's not used in mainline any more.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

I think it would make sense to deprecate support for [unit]overlay=, and then verify with the Schema validator that it's not used in mainline any more.

Hmm while we could in theory do this, this could mean i behaves quite differently from all other attributes which can be set from both effects and via [unit] attribute (alkthough i'd say you usually shodul prefer the [effect] way).

@gfgtdf gfgtdf force-pushed the gfgtdf:fix_remove_overlay branch from 701ecab to 3f195ea Aug 10, 2019

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

updated the is with changes from wesnoth:master, now i have to rmember what i wanted to do here next

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Confused, isn't this obsoleted by #4211? Am I missing something here?

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Hmm have to think about it in particular whether I still want to do the change to the loyal trait.

stevecotton referenced this pull request Aug 12, 2019

Handle unit overlays as modifications, allow [effect] to remove them
Possible fix for #4058, with the following logic:

* If non-empty, [unit]overlay= is handled by adding modifications
* unit::write will always output an empty overlay=
* The Lua API's get_units() will still provide the list of overlays
* [effect]apply_to=overlay can now remove as well as add overlays
* [remove_unit_overlay] is implemented with [effect]apply_to=overlay

Using [object]s with durations hasn't been tested, but expected effects:
* An expired add= followed by a non-expired remove= will simply cause the remove=
    to have no effect when std::remove(overlays_ ...) is called.
* A remove= followed by [remove_unit_overlay] cause the [remove_unit_overlay] to be a no-op,
    and the overlay will reappear when the first remove= expires. This edge case is already
	documented as unsupported on the wiki.
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

[remove_trait] is passed trait_id but is checking for object_id.

@gfgtdf gfgtdf force-pushed the gfgtdf:fix_remove_overlay branch from d1e3848 to 2e27596 Aug 16, 2019

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

i want to add a duration= key to [unit_overlay] that basiciall sets the duration of the object. any opinion whethe it should be called duration= or object_duration=

@stevecotton

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

I'd prefer duration=, because:

  • without the knowledge that [unit_overlay] is implemented with [object], there's no reason to expect object_duration=
  • with that knowledge, it makes sense to be the same key as is used in [object]
@@ -964,10 +969,10 @@
find_vacant=no
[/unstore_unit]

[remove_unit_overlay]
[remove_object]

This comment has been minimized.

Copy link
@stevecotton

stevecotton Aug 16, 2019

Contributor

If the overhead of remove_unit_overlay is so high that it needs to be changed to remove_object, then that feels more like a bug in remove_unit_overlay than something to change in campaigns' files.

This comment has been minimized.

Copy link
@gfgtdf

gfgtdf Aug 16, 2019

Author Contributor

i don't think it's a problem real problem it just feels better knowing that there are no useless objects laying around in the unit.

This comment has been minimized.

Copy link
@stevecotton

stevecotton Aug 16, 2019

Contributor

If it's worth doing, I'd prefer it to be done in a way that benefits UMC too. Maybe in the unit.cpp's expire_modifications.

I won't have time until Sunday, but would be happy to do the C++ implementation on Sunday.

This comment has been minimized.

Copy link
@gfgtdf

gfgtdf Aug 16, 2019

Author Contributor

If it's worth doing, I'd prefer it to be done in a way that benefits UMC too. Maybe in the unit.cpp's expire_modifications.

i don't think its worth doing this in the c++ code, i mean i only possibility i see is to search for objects that cancel each other out but, then that'd break umc code that relied on such objects and wanted to remove the second object manually via [remove_object].. . I just don't think there is a good way. These wml changes i did were cheap and without disadvantages that's why i did them, changing it in the c++ code has the chance to break other code and for a minimal gain of reducing savefile size of a few bytes.

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Aug 17, 2019

Member

I think it should:

  1. Make [remove_unit_overlay] be no worse than [remove_object].
  2. Deprecate [remove_unit_overlay] (possibly also [unit_overlay]).
  3. Replace all mainline uses of [remove_unit_overlay] with [remove_object].

This comment has been minimized.

Copy link
@stevecotton

stevecotton Aug 17, 2019

Contributor

If we don't deprecate the tag, then I think it should still be used in mainline, otherwise it won't be tested by playing the mainline campaigns.

@gfgtdf gfgtdf force-pushed the gfgtdf:fix_remove_overlay branch 2 times, most recently from e2d53d2 to 901661b Aug 16, 2019

gfgtdf added some commits May 20, 2019

utbs: use [object] for dehydration implementation
this in particular makes it compatible the umc code that uses [remove_object]
reset ellipse on unit advancing
same reason as for the overlays= change, in particular this fixes
remove_object of objects that set ellipses.
add [unit_overlay] object_id= attribute
with this the object can be removed with [remove_object]

gfgtdf added some commits Aug 16, 2019

use [remove_object] over [remove_unit_overlay] if possible
this just prevents the units wml from getting cuttered with many [objects]
(generated by the [unit_overlay] implementation) that cancel each other out.
(in the cases here it shouldn't really be a problem though, i still did it
because it feels clener this way)
move loyal overlays to the loyal trait
this in particular fixes #4058 . Also people simply forgetting the
IS_LOYAL macro was a not-so-uncommon cause of bugs.

In some cases people want to give the LOYAL trait without giving the
loyal overlays for those cases i added a optional OVERLAY parameter to
the {TRAIT_LOYAL} macro, also heroes should now use {TRAIT_LOYAL_HERO}
instead which uses the hero overlay. The IS_LOYAL and IS_HERO macros are
no longer needed in combination with those traits.

@gfgtdf gfgtdf force-pushed the gfgtdf:fix_remove_overlay branch from 901661b to e1f7def Aug 16, 2019

[/modify_unit]
#enddef

#define MAKE_LOYAL_HERO IS_STRING

This comment has been minimized.

Copy link
@stevecotton
@@ -135,6 +136,11 @@ local function simple_modify_unit(cfg)
end
u:add_modification(tagname, tagcontent);
end
if tagname == "status" then

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Aug 17, 2019

Member

Why is this needed? Isn't it handled by the general case?

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