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

Replace deprecated share_maps and share_view with share_vision #6498

Merged
merged 2 commits into from Feb 15, 2022

Conversation

Wedge009
Copy link
Member

@Wedge009 Wedge009 commented Feb 8, 2022

Closes #6496.

Not sure if I'm understanding the documentation correctly but I interpreted share_maps=yes as share_vision=shroud and share_view=yes as share_vision=all.

Also not sure if it's worth back-porting, since there's the potential to break compatibility for games in progress. But if reviewers think it's okay, I can back-port too.

@Wedge009 Wedge009 added Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign Unit Tests Issues involving Wesnoth's unit test suite. WML Issues involving the WML engine or WML APIs. labels Feb 8, 2022
@Wedge009
Copy link
Member Author

Wedge009 commented Feb 8, 2022

Hmm, where are those ...share_view tests defined? Did it break just because I renamed the tests to match the updated code?

@stevecotton
Copy link
Contributor

The file wml_test_schedule.

Copy link
Member

@knyghtmare knyghtmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were all the edited scenarios checked to see if the update works as intended? Otherwise we might be preparing for a flood of bugs.

@knyghtmare
Copy link
Member

Also, it goes like this:
if share_maps (it means shroud data), so just share_vision=shroud
if share_view (it means fog), so share_vision=all
Share_vision has no specific value for "just FoW" so in those cases it should be set for "all".

In any case, the edits look fine.

@Wedge009
Copy link
Member Author

Wedge009 commented Feb 8, 2022

Were all the edited scenarios checked to see if the update works as intended?

I checked the test scenario and a few of the campaign scenarios and didn't see anything obviously wrong but I'd still prefer for others to check this as I'm not entirely sure of 'correct' behaviour.

Also, it goes like this: if share_maps (it means shroud data), so just share_vision=shroud if share_view (it means fog), so share_vision=all Share_vision has no specific value for "just FoW" so in those cases it should be set for "all".

That's basically how I read it too.

@stevecotton
Copy link
Contributor

All looks good to me. I haven't tested in-game.

@Wedge009
Copy link
Member Author

Should I wait till someone else does some testing before merging?

@stevecotton
Copy link
Contributor

I've tested all the campaign changes except for the LoW ones. The LoW ones look right, but as it's getting rewritten anyway I didn't see the need to test.

My feeling is to merge, but as it's campaign content I think the decision has to be made by @nemaara rather than me.

Copy link
Contributor

@nemaara nemaara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upcoming rework has no bearing on making improvements to the current version.

@stevecotton
Copy link
Contributor

@Wedge009 I've added a few comments about possibly adding comments, but feel free to merge as-is or with those as you feel appropriate.

Closes wesnoth#6496.

share_maps=yes -> share_vision=shroud
share_view=yes -> share_vision=all
@Wedge009
Copy link
Member Author

@stevecotton Thanks for your review. If you can just double-check the second commit relating to the sharing of Chantal's side's vision and advise whether or not it's okay, then I'll merge or correct accordingly.

@Wedge009 Wedge009 merged commit 4c88909 into wesnoth:master Feb 15, 2022
@Wedge009 Wedge009 deleted the share_vision branch February 15, 2022 11:08
@stevecotton
Copy link
Contributor

I think it makes sense to backport all of this. Now we've tested DM S11 on 1.16, I'm confident that these changes won't introduce any bugs.

@Wedge009
Copy link
Member Author

Back-ported with 80c9263 + c80f83d + 28ce494.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign Unit Tests Issues involving Wesnoth's unit test suite. WML Issues involving the WML engine or WML APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LoW] [deprecated code] replace share_view=yes in favour of share_vision=all
4 participants