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

WML: Support [filter_side] in [item]. #3533

Merged
merged 4 commits into from Sep 22, 2018

Conversation

Projects
None yet
3 participants
@jostephd
Member

jostephd commented Sep 8, 2018

If [filter_side] is present then "team_name" is ignored.

Fixes #1477.

Constructing a comma-separated set of team names is ugly but I didn't want to change add_overlay and the overlay struct.

wesnoth/src/overlay.hpp

Lines 23 to 26 in cc98bcd

overlay(const std::string& img, const std::string& halo_img,
halo::handle handle, const std::string& overlay_team_name, const std::string& item_id, const bool fogged) : image(img), halo(halo_img),
team_name(overlay_team_name), id(item_id), halo_handle(handle) , visible_in_fog(fogged)
{}

WML: Support [filter_side] in [item].
If [filter_side] is present then "team_name" is ignored.

Fixes #1477.
@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Sep 8, 2018

Member

Just wondering, but... this doesn't fully support #1477, does it? I mean, if sides 1 and 2 are allied, it won't let you make a label visible only to side 1, right?

I'm not saying this is a problem, but...

Member

CelticMinstrel commented Sep 8, 2018

Just wondering, but... this doesn't fully support #1477, does it? I mean, if sides 1 and 2 are allied, it won't let you make a label visible only to side 1, right?

I'm not saying this is a problem, but...

@jostephd

This comment has been minimized.

Show comment
Hide comment
@jostephd

jostephd Sep 8, 2018

Member

Good point. Items are per-team and this PR does not make them per-side. I could change the tag to [item][filter_team] instead of [item][filter_side] to make that clear?

Member

jostephd commented Sep 8, 2018

Good point. Items are per-team and this PR does not make them per-side. I could change the tag to [item][filter_team] instead of [item][filter_side] to make that clear?

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Sep 8, 2018

Contributor

the current 'per team' implementation is also somwhzow bas in particular it doesnt play well with the current teams alliances syntax: the follwing example:

side1 is in team team1
side2 is in team team2
side3 is in team team1,team2 (allied to both)

with the current syntax it is afaik not possibel to create a label that is visible for 'team1' that is side1 and side3 but not for side2, this is because the check in display used substring, so visibility to team1,team2 imples visibility to team1 while actually the opposite way would make more sense. Of yourse this can be fixed, but maybe it makes more sense to change it to just sote the sides instead.

Another implementation woudl be to, instead of storeing sides ot team in the the overlay object to store the compltete filter in the overlay object and evaluate it when the item is drawn, this way the items visiblitity would (actuall this might not be true, it very much depnds on how exactly the drawing code works) automaticially be updated when the properties of the side change.

Contributor

gfgtdf commented Sep 8, 2018

the current 'per team' implementation is also somwhzow bas in particular it doesnt play well with the current teams alliances syntax: the follwing example:

side1 is in team team1
side2 is in team team2
side3 is in team team1,team2 (allied to both)

with the current syntax it is afaik not possibel to create a label that is visible for 'team1' that is side1 and side3 but not for side2, this is because the check in display used substring, so visibility to team1,team2 imples visibility to team1 while actually the opposite way would make more sense. Of yourse this can be fixed, but maybe it makes more sense to change it to just sote the sides instead.

Another implementation woudl be to, instead of storeing sides ot team in the the overlay object to store the compltete filter in the overlay object and evaluate it when the item is drawn, this way the items visiblitity would (actuall this might not be true, it very much depnds on how exactly the drawing code works) automaticially be updated when the properties of the side change.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Sep 8, 2018

Member

I'd say that's a separate situation to this @gfgtdf; but assuming we stick to specifying team rather than side, the correct way to determine if a label is visible to a side would be to split the label's team name and the side's team name and perform a set intersection. If it's empty, the label is not visible.

Member

CelticMinstrel commented Sep 8, 2018

I'd say that's a separate situation to this @gfgtdf; but assuming we stick to specifying team rather than side, the correct way to determine if a label is visible to a side would be to split the label's team name and the side's team name and perform a set intersection. If it's empty, the label is not visible.

jostephd added some commits Sep 8, 2018

WML: Compare [item]team_name to [side]team_name using intersection.
Fixes problems with substrings and when one or the other is a
comma-separated list. See #3533
@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Sep 8, 2018

Member

So basically this ends up taking the union of the matched side's teams, right? (Or concatenation technically, but in effect it should come out the same.)

Member

CelticMinstrel commented Sep 8, 2018

So basically this ends up taking the union of the matched side's teams, right? (Or concatenation technically, but in effect it should come out the same.)

@jostephd

This comment has been minimized.

Show comment
Hide comment
@jostephd

jostephd Sep 8, 2018

Member

Yes. intf_add_tile_overlay will call add_overlay with team_name set to the union of all teams that at least one matched side is on.

Member

jostephd commented Sep 8, 2018

Yes. intf_add_tile_overlay will call add_overlay with team_name set to the union of all teams that at least one matched side is on.

@CelticMinstrel CelticMinstrel merged commit 3a3b752 into wesnoth:master Sep 22, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jostephd added a commit to jostephd/wesnoth that referenced this pull request Oct 6, 2018

WML: Support [filter_side] in [item]. (wesnoth#3533)
* WML: Support [filter_side] in [item].

If [filter_side] is present then "team_name" is ignored.

Fixes wesnoth#1477.

* WML: Compare [item]team_name to [side]team_name using intersection.

Fixes problems with substrings and when one or the other is a
comma-separated list. See wesnoth#3533

* WML: Rename [item][filter_side] to [item][filter_team]

* Add changelog entry

jostephd added a commit to jostephd/wesnoth that referenced this pull request Oct 7, 2018

WML: Support [filter_side] in [item]. (wesnoth#3533)
* WML: Support [filter_side] in [item].

If [filter_side] is present then "team_name" is ignored.

Fixes wesnoth#1477.

* WML: Compare [item]team_name to [side]team_name using intersection.

Fixes problems with substrings and when one or the other is a
comma-separated list. See wesnoth#3533

* WML: Rename [item][filter_side] to [item][filter_team]

* Add changelog entry

(cherry-picked from commit 3a3b752)

@jostephd jostephd referenced this pull request Oct 7, 2018

Closed

Working master branch #3603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment