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

Misc more api #3992

Open
wants to merge 5 commits into
base: master
from

Conversation

@gfgtdf
Copy link
Contributor

commented Mar 17, 2019

No description provided.

@ProditorMagnus
Copy link
Contributor

left a comment

persitent -> persistent

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

  • item_name - Fine, but you need to rebase and run wmlindent on this commit
  • persistent - Fine, I see no problems
  • [unit_type_fix] - Seems okay, but I'd like to see it supported also in [campaign]. Also, I suggest adding a set_cost key for UtBS.
  • [modify_unit][set_variable] - Well, this doesn't really add anything new that you couldn't do before, does it? But I guess it is a more convenient syntax than using [modify_unit][variables]key=$(some_formula), so sure.
@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

persitent -> persistent

will fix thx

* `[unit_type_fix]` - Seems okay, but I'd like to see it supported also in `[campaign]`. Also, I suggest adding a `set_cost` key for UtBS.

It is already supported in campaign, will update the commitmessage. Also set_cost sounds like a good idea.

* `[modify_unit][set_variable]` - Well, this doesn't really add anything new that you couldn't do before, does it? But I guess it _is_ a more convenient syntax than using `[modify_unit][variables]key=$(some_formula)`, so sure.

Hmm i should maybe support clear_varible there aswell if i have set_variable (which is something [modify_unit][variables] cannot do afaik, in particular removing whole subtags tags). Honestly my main motive here was to make variable changing compatible with the [modify_unit] optimisation in

local function simple_modify_unit(cfg)
and allowing set_varible was not only easier to implement (since i can just call the original set_variable code as i did) but also a little more convinient to use.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I don't think [modify_unit] supports the full merge syntax, so yeah, it probably can't remove keys or tags. That doesn't matter much for the root level, where the valid set of keys is fixed, but in variables it's kinda limiting; maybe we should add a wml.merge() function to merge two configs (same semantics as in [set_variables] using merge mode), and make use of it in [modify_unit]?

Regarding set_variable_impl, I'd say you don't actually need it. You can just add the extra parameter directly to the wml function, like this:

function wesnoth.wml_actions.set_variable(cfg, variables)
    variables = variables or wml.variables
    -- remainder of the function stays as it is
end
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Also, if we're supporting [set_variable] and maybe [clear_variable] in [modify_unit], then why not also in [modify_side]?

@gfgtdf gfgtdf force-pushed the gfgtdf:moreapi branch from cc71d18 to eb1ce67 Mar 18, 2019

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Also, if we're supporting [set_variable] and maybe [clear_variable] in [modify_unit], then why not also in [modify_side]?

hmm we could, but is there a direct (=without lua) way to retreive those varibles in wml in the first place? If not there is little point is being able to set them.

Regarding set_variable_impl, I'd say you don't actually need it. You can just add the extra parameter directly to the wml function, like this:

Hmm yes is this really better than having a seperate function? Adding another argument to a wml tag that cannot be used from wml seems sounterintuitive to me, but i don't really have a strong opinion on this.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

hmm we could, but is there a direct (=without lua) way to retreive those varibles in wml in the first place? If not there is little point is being able to set them.

Well, how does one retrieve unit variables in WML? Through [store_unit], probably? So maybe side variables would be available through [store_side]? (If not maybe they should be.)

Hmm yes is this really better than having a seperate function? Adding another argument to a wml tag that cannot be used from wml seems sounterintuitive to me, but i don't really have a strong opinion on this.

It's certainly debatable whether it's better than having a separate function. However, it's definitely better than polluting the wesnoth table with an implementation detail. If you prefer a separate function, an alternative might be to declare the implementation function local but return it from the module, so you can get it with wesnoth.require "wml/set_variable" when you need it elsewhere.

gfgtdf added some commits Jan 1, 2019

support item_name= in [store_items]
we use it in SotA16 to make the scenario more robust against
[modification]s that might add [item]s for decoration.
add [unit_type_fix] in [scenario], [era] etc
fixes #1451

Currently advancements can be changed in two ways:
1) Via [advancefrom], this causes OOS in multiplayer
2) Via certain campaign etra defines, this has other disadvantages see

The new [unit_type_fix] that is accepted in [scenario], [multiplayer],
[era], [modification] , [campaign] and [ressource] changes the advancement
of a unit type only for one game and has non of the disadvantages of
the other two methods.

We should also deprecate the other two methods.
Move split_foreach to string_utils.hpp
Split foreach is a little faster than regular split since it does not create
temporary std::string or std::vector objects

This also optimizes the regular split since it no longer creates
a temporary stringstream object.

A nice alterntive to split_foreach could be boost::algorithm::split_iterator
but that one returns iterator_range objects instead of string_view objects.
add [modify_unit][set_variable]
[set_variable] can now be used inside [modify_unit]  to directly modify
the units variables, enabling [modify_unit] to replace some more
store+unstore_unit codes.

@gfgtdf gfgtdf force-pushed the gfgtdf:moreapi branch from 605bc71 to c89a7ec Mar 20, 2019

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

merged the first 3 commits to master.

@jostephd

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Thanks! Please remember to update the wiki as well:

daf0547
3c172f7
e4c4ca7

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

  • Regarding the todo note in [store_items], I'd recommend a comma-separated list, not a regex. Nothing else in the main Wesnoth schema uses regex as far as I know (the schema does, but that's a separate, well, schema). If you did want to support regex, I'd recommend making it a separate key (eg, item_regex).
  • You still have wesnoth.set_variable_impl; please choose some other way to do this. The wesnoth table should not be polluted with implementation details. I've already suggested two alternative ways to handle this.
@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I think i might postpone the [modify_unit][set_variable] until i actually need it.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

By the way, these new features need to be added to the schema, too.

@sevu sevu added this to the 1.15.0 milestone Mar 23, 2019

@gfgtdf gfgtdf added the Postponed label Apr 1, 2019

@jostephd

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

@gfgtdf Reminder that 1.15.0 is in two weeks (this issue is milestoned 1.15.0).

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Reminder that 1.15.0 is in two weeks

Hmm yes I don't think I'll merge this one before, it's also marked as postoned

Did we already decide on what do do with 1.15 will it be a usual Dev release or did we change our release system? I'm not really active on irc/discord currently the irc client on my phone is not really good and discord hates me for some reason

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.