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

support scenario files in map_file #4804

Merged
merged 2 commits into from
Mar 15, 2020
Merged

Conversation

gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented Mar 14, 2020

Now a scenario generated by teh scenario editor can be loaded
just like a simple map file
fixes #4803

Now a scenario generated by teh scenario editor can be loaded
just like a simple map file
@CelticMinstrel
Copy link
Member

Can you explain how the merge works for this case? Will it be different from macro-including the scenario file?

I kinda question the addition of inherit_attributes to the config API. Do you think there are other use-cases for it?

@gfgtdf
Copy link
Contributor Author

gfgtdf commented Mar 14, 2020

Can you explain how the merge works for this case?

Yes, is basically like the inclusion,except that it tries to merge [side] tags, so that you can still overwrite the contents of each [side] in the handwritten .cfg file, but still create units in the scenario editor.

Do you think there are other use-cases for it?

Well, one usecase is enough. Its clearly a function that does a basic config operation so it belongs in config.hpp and fits well along the existing append/inherit etc functions.

@CelticMinstrel
Copy link
Member

Its clearly a function that does a basic config operation

Well, this is precisely what I was questioning…

Also, isn't it identical to the existing merge_attributes?

@gfgtdf
Copy link
Contributor Author

gfgtdf commented Mar 14, 2020

We coudl improve the merging for exampy by also marking it merge the [unit]s in each [side], correctly, but im not convonced that this actually better than makring people use something liek [modify_unit]+prestart events.

@gfgtdf
Copy link
Contributor Author

gfgtdf commented Mar 14, 2020

Also, isn't it identical to the existing merge_attributes?

no, inherit_attributes choose the already existent attributes over the new ones, but the existing functions config::append_attributes and config::merge_attributes do basicially the same thing.

@CelticMinstrel
Copy link
Member

I think merging the units in each side would be more confusing than just appending them.

Also, did you miss my question just above?

@CelticMinstrel
Copy link
Member

no, inherit_attributes choose the already existent attributes over the new ones

Sure, but doesn't that just mean that x.inherit_attributes(y) is equivalent to y.merge_attributes(x)? Or maybe append_attributes.

@gfgtdf
Copy link
Contributor Author

gfgtdf commented Mar 14, 2020

Sure, but doesn't that just mean that x.inherit_attributes(y) is equivalent to y.merge_attributes(x)? Or maybe append_attributes.

No, these function all change the this object, not the passes config object

@CelticMinstrel
Copy link
Member

No, these function all change the this object, not the passes config object

I don't quite get how this makes them not equivalent? Whichever object ends up changed, the operation is the same, right? In fact, isn't inherit_from literally copy-merge-swap?

@gfgtdf
Copy link
Contributor Author

gfgtdf commented Mar 14, 2020

I don't quite get how this makes them not equivalent?

No, on both cases you end up with a config that has x values with y as fallback, but in one method you'll also end up with another config with x values and with the other method you'll end up with another config with y values.

In fact, isn't inherit_from literally copy-merge-swap?

So? Obviously merge is not the same as copy-merge-swap

@gfgtdf gfgtdf merged commit 528dbcc into wesnoth:master Mar 15, 2020
@CelticMinstrel
Copy link
Member

No, on both cases you end up with a config that has x values with y as fallback, but in one method you'll also end up with another config with x values and with the other method you'll end up with another config with y values.

This means they are equivalent if you first make a copy of the LHS config. So if nothing else, the inherit_attributes function should probably be implemented in terms of one of the other functions to reduce code duplication and reflect this equivalency.

@gfgtdf
Copy link
Contributor Author

gfgtdf commented Mar 15, 2020

So if nothing else, the inherit_attributes function should probably be implemented in terms of one of the other functions to reduce code duplication and reflect this equivalency.

I disagree, and I doubt your suggested implemtation would be noticable smaller or easier to read, but if you want you can do it.

@Wedge009 Wedge009 added the Engine General game engine issues that do not fit in any other category. label Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engine General game engine issues that do not fit in any other category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support scenario files in [scenario]map_file
3 participants