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

mp_create_game: Show preview if map_file is set #4407

Merged
merged 3 commits into from Oct 10, 2019

Conversation

@jostephd
Copy link
Member

commented Sep 30, 2019

Fixes the second bullet of #4397 (comment).

Could someone double check the change to the generator_assigned condition? Is it about the case that a scenario file defines both map_file and a map generator? I would have expected that to be an error, actually.

@jostephd jostephd requested a review from sevu Sep 30, 2019
@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

Could someone double check the change to the generator_assigned condition?

Is it about the case that a scenario file defines both map_file and a map generator?

yes.

The result of the map generator is stored in the map_data attribute , so that code checks whether the map was already generated and does so if it didnt happen yet.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

from looking at the code im suprised this works, since all codes you are changing are inside type == ng::level::TYPE::RANDOM_MAP blocks, and i'd expect that the normal mp maps are not 'random maps'. In fact random maps are the only type of maps where map_file does not make sense.

@jostephd jostephd self-assigned this Sep 30, 2019
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

@gfgtdf That branch of the switch statement handles all of the following:

		case ng::level::TYPE::SCENARIO:
		case ng::level::TYPE::USER_MAP:
		case ng::level::TYPE::USER_SCENARIO:
		case ng::level::TYPE::RANDOM_MAP: {
@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

@gfgtdf That branch of the switch statement handles all of the following:

right, forogt how C++ switch works for a second.

but don't we use FALLTHROUGH for intentional fallthough switches and don't he compiler give a wanring for missing FALLTHROUGH?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

but don't we use FALLTHROUGH for intentional fallthough switches and don't he compiler give a wanring for missing FALLTHROUGH?

Compilers probably suppress that warning when a label is followed by another label with no statements in between. Anyway, falling through happens whenever there's no break; statement; comments don't affect control flow.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

By the way, I wanted to put somewhere an accessor function, foo::map_data() that just gets the map_data from either the map_data attribute or the map_file attribute, whichever exists, and use that accessor function in all three places that currently access "map_file"...but I couldn't figure out where to put the new function. The three places that handle map_file don't seem to even have a common #include.

@soliton-

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

Factoring the code out to a common function sounds good. Not sure where to put it either though.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

I could just create a non-member function somewhere and have the three places use it. Even if that's not the best possible place to declare the function in, we can move it to a more suitable home when we come up with one.

CC @Vultraz

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

well at least one of three function that use map_file don't 'access' it they, read map_file and store it into map_data. So not sure how muhc there is in common actually.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

well at least one of three function that use map_file don't 'access' it they, read map_file and store it into map_data. So not sure how muhc there is in common actually.

I thought of a function that takes a config object and returns a string. That string will be c.has_attribute("map_file") ? filesystem::read_map(c["map_file"]) : c["map_data"]. All three locations can use that, I think.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

hmm no the saved_game code does more like if c.has_attribute("map_file") then c["map_data"] = read_map(c["map_file"]) end return c["map_data"]; and in fact i think that'd also make more sense for the mp_create code so that the map_file is not read twice.

jostephd added a commit to jostephd/wesnoth that referenced this pull request Oct 1, 2019
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

and in fact i think that'd also make more sense for the mp_create code so that the map_file is not read twice.

Done

Copy link
Member

left a comment

Tested and works

@@ -4,7 +4,7 @@
name= _ "2p — Aethermaw"
# wmllint: local spellings Sulla Aethermaw Paterson
description= _ "Long ago, the Great Mage Sulla was imprisoned in the Aethermaw, a nexus of mystical energy whose chaotic nature was able to prevent her escape. Over the centuries, however, Sulla gradually attuned her powers to the maelstrom of disorder that is the Aethermaw, and has now begun to project its influence onto the material plane, drawing in entire regions of land from hundreds of different worlds, realities and time-periods. She experiments with these disparate pieces of the cosmos, manipulating them, merging them and sending them back and forth between the Aethermaw and their place of origin. Perhaps, as her mastery over the Aethermaw grows, Sulla will one day break free of its bonds. Until that time comes, she will continue to amuse herself by arranging battles between the mortal beings unlucky enough to be drawn into its depths. Designed by Doc Paterson."
map_data="{multiplayer/maps/2p_Aethermaw.map}"
map_file=multiplayer/maps/2p_Aethermaw.map

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 10, 2019

Author Member

error validation: Invalid key 'map_file=' in tag [multiplayer]

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 10, 2019

Author Member

The rebase should fix this.

@jostephd jostephd force-pushed the jostephd:mp-create-game-map_file branch from 67dd920 to d9d794d Oct 10, 2019
@jostephd jostephd merged commit eccfba6 into wesnoth:master Oct 10, 2019
2 of 3 checks passed
2 of 3 checks passed
label label
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jostephd added a commit that referenced this pull request Oct 10, 2019
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.