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

resolve Check whether special is active by tag name or by id #3920

Open
wants to merge 14 commits into
base: master
from

Conversation

@newfrenchy83
Copy link
Contributor

commented Feb 10, 2019

resolve #3915 with add of special_id and special_tags( and idem for special_active)

newfrenchy83 added some commits Feb 10, 2019

Check whether special is active by tag name or by id
resolve #3915 with add of special_id and special_tags( and idem for special_active)

newfrenchy83 added some commits Feb 11, 2019

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

@jostephd could you test this PR please?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

I didn't look at it in detail, but it seems to me that there is quite some space for refactoring/ removing code dublication.

newfrenchy83 added some commits Feb 24, 2019

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

i remove code duplication.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@gfgtdf code duplication corrected and sorry for the late.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

This seems kind of strange. Why do we now have three different keys? We have special, special_id, and special_tags. Aren't there only two ways to test a special? It can match by tag or by ID, right? Doesn't special already go by tag? I see its code is slightly different from that for special_tags, but is there any actual semantic difference?

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

If i filter drains special with special key then the filter weapon with drains id but also with drains tags, with special_id i match id=drains only but not the orher drains tags weapon. But yes i can remove special_tags and use what special_id and special.

@jostephd

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

I think what happens is that special=foo tries to match foo both as an id and as a tag name, and then special_id=bar and special_tags=baz try to match bar only as an id and baz only as a tag. In which case the right thing is probably to support all three but mark special=foo as deprecated (https://wiki.wesnoth.org/CompatibilityStandards).

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

I am of the same opinion as @jostephd concerning special = foo, and it is better to have special_id on the one hand and special_tags on the other hand to avoid filtering a type of special attack when we only target only one in this type.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

For consistency with ability filtering in StandardUnitFilter, I'd say you should have special= that matches on ID and special_type that matches on tag name. Unfortunately that's problematic with respect to backwards compatibility... but you can at least rename your special_tags to special_type.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I probably should've mentioned this explicitly before, but... again for consistency, please use special_type_active instead of special_active_type. Similarly, I'd prefer special_id_active instead of special_active_id.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Just wondering, regarding deprecation... would it be acceptable to deprecate special and special_active for now, and after a certain period, change its behaviour to match special_id and special_id_active? Then, perhaps after another period of time, undeprecate the first two and deprecate the second two.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@CelticMinstrel This would mean that special_id should be invalidated after inserting it into the wml code with special_type so that special, at that time , but at the same time modify the behavior of special would prevent develloper from reworking the code, view that all or almost use special to refer to the id of the special weapon and not the type, in this respect yes, your proposal is acceptable to use special_id transiently .

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@CelticMinstrel have you forget this PR?

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I did, actually.

I don't see any problems to prevent this being merged at this point.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@CelticMinstrel this PR will be commited a day?

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

I think it can probably be merged, yes. I'd rather not be the one to do it though, unless there's several people here saying the same thing.

@jostephd @gfgtdf - what do you think?

@jostephd

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

I don't have time to review this currently, but I'll take a look when I have time for wesnoth again.

newfrenchy83 added some commits May 10, 2019

replace special_id by special
instead to put a "special_id" specifically for the specials id i finally to modify "special" and "special_active" for it refer to id only ans use "special_type" for refer to special type.
@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@CelticMinstrel 2 things: primo i covnerted special for filter id only and remove special_id. secundo nor @jostephd or @gfgtdf have review this PR.

@jostephd jostephd added this to the 1.16.0 milestone May 29, 2019

@jostephd

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Set milestone.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Listen, i know what you're many PR to review, but please don't forget this PR.

@Wedge009

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Just looking at the code, it seems to make sense to me. I'll try to test when I get home - what units make use of weapon specials? Actually, never mind, I just saw #3915 referring to the first strike ability in HttT.

@CelticMinstrel Any particular reason for the reticence to merge or just preferring to let @jostephd check first?

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.