New relaunch rule

Not logged inOpenClonk Forum

Forum Home Help Search Watchlist Register Login

Topic Development / Scenario & Object Development / New relaunch rule

1 2

Post

By Fulgen

Date 2017-04-13 17:34

There have been changes to the relaunch system: There is now a new relaunch rule which handles relaunches, general respawns and player restarts. Due to this, it will be necessary to adapt the scenarios (all scenarios included in the main package have been updated).

Major compatibility issues (functions are all methods of the object returned by GetRelaunchRule())
* Rule_Restart needs to be replaced by EnablePlayerRestart()
* Rule_BaseRespawn needs to be replaced by SetBaseRespawn(true)
* FreeCrew and InventoryTransfer can be set by calling SetInventoryTransfer(true/false) and SetFreeCrew(true/false)
* Remove all existing respawn code in the scenario, it isn't needed any more.
* If the game mode is melee, the default relaunch count is 5, otherwise, it is nil (unlimited relaunches). You can set it with SetDefaultRelaunches(relaunches).

By Zapper

Date 2017-04-13 18:30

Why? There's no explanation in the commit

By Fulgen

Date 2017-04-13 19:01

>The relaunch rule could automatically create the clonk release rule (which allows you to restart from the rule menu). Alternatively, the two rules could just be merged into one. Scenario designers always forget to add that rule.

By Clonkonaut [de]

Date 2017-04-21 10:28 Edited 2017-04-21 10:34

Missions (at least the campaign ones) are broken. I just tested Crash and Dark Castle. Apart from errors that happen, the default relaunch mechanic of your rule seem to be 'randomly spawn in the landscape'. That's basically what happens there.

Also, your inventory doesn't get transferred.

In general, I'd appreciate a function to disable respawning of script players. That's the addition I had to make in my campaign.

By Fulgen

Date 2017-04-21 17:05

o.O
I'm going to fix that this weekend and implement the named function.

By Clonkonaut [de]

Date 2017-04-21 10:40

Also broken are the arenas with the weapon chooser (Overcast, Rock Bottom, Thunderous Skies, Molten Monarch). Especially Overcast just spams errors.

By Fulgen

Date 2017-04-21 17:04

The weapon chooser is already fixed in my fork. Overcast didn't spam any error when I played it with K-Pone and Foaly.

By Clonkonaut [de]

Date 2017-04-21 22:24

I suggest doing your test runs in the editor and in single player, not some random multiplayer game (you might miss quite a lot of errors because of the disabled debug mode). As for the errors, I get these:

ERROR: symbol not found in any symbol table: pos (in Scenario.Prototype.OnClonkLeftRelaunch, Arena.ocf\Overcast.ocs\Script.c:343:31)
ERROR: symbol not found in any symbol table: pos (in Scenario.Prototype.OnClonkLeftRelaunch, Arena.ocf\Overcast.ocs\Script.c:343:38)

by: Scenario->OnClonkLeftRelaunch(Object(448)) (Arena.ocf\Overcast.ocs\Script.c:0)
ERROR: syntax error: symbol not found in any symbol table: pos (in Scenario.Prototype.OnClonkLeftRelaunch, Arena.ocf\Overcast.ocs\Script.c:343:31)
 by: Object(460)->GameCall("OnClonkLeftRelaunch",Object(448)) (System.ocg\Action.c:36)
 by: Object(460)->RelaunchClonk() (Objects.ocd\Rules.ocd\Relaunch.ocd\RelaunchContainer.ocd\Script.c:115)
 by: Object(460)->FxIntTimeLimitTimer(Object(460),effect {Name = "IntTimeLimit"},36) (Objects.ocd\Rules.ocd\Relaunch.ocd\RelaunchContainer.ocd\Script.c:79)

By Maikel Date 2017-04-22 14:34 Edited 2017-04-24 18:02

[15:30:49] ERROR: can't access nil as array or proplist
[15:30:49] by: Object(239)->DoRelaunch(0,0,0,true) (/home/maikel/openclonk/repos/build/planet/Objects.ocd/Rules.ocd/Relaunch.ocd/Script.c:292)
[15:30:49] by: Object(239)->InitializePlayer(0,319,264) (/home/maikel/openclonk/repos/build/planet/Objects.ocd/Rules.ocd/Relaunch.ocd/Script.c:175)

By Maikel Date 2017-04-24 18:02

What is the status on the remaining issues?

By Fulgen

Date 2017-04-24 18:03

Fixed in a pull request.

By Maikel Date 2017-04-25 08:26

In what pull request? I merged this and got an error:

ERROR: symbol not found in any symbol table: respawn_script_players (in Rule_Relaunch.OnClonkDeath, /home/maikel/openclonk/repos/build/planet/Objects.ocd/Rules.ocd/Relaunch.ocd/Script.c:183:33)

By Maikel Date 2017-04-25 08:48

Fixed the error above and merged it into master.

By Clonkonaut [de]

Date 2017-04-25 09:54

Missions are still broken. They all use a 'Set' function of the relaunch rule that doesn't exist. Unfortunately, the repository is still not in a working state.

By Maikel Date 2017-04-25 09:55

Indeed, Fulgen could you please ensure that you test every single commit from now on, they all should be error free by themselves.

By Fulgen

Date 2017-04-25 12:56 Edited 2017-04-25 13:03

Can't be, I committed this Set function...
Okay, this is odd. The commit on the branch fulgen-master has the Set() - function, after the cherry-pick, it didn't.

By Maikel Date 2017-04-25 13:11

Another argument for making commits that work by themselves!

By Maikel Date 2017-04-25 13:27

Is only the Set function missing?

And what is the reason for using both function chaining and proplist setting? What is wrong with calling a series of setters?

By Maikel Date 2017-04-30 12:16

Reminder: this still is not fixed. I prefer not having this set functions and using the old system, any objection to me removing set and using normal Set* functions?

By Clonkonaut [de]

Date 2017-04-30 12:24

Comparing the two scripts, there are more differences than just a missing Set function.

By Fulgen

Date 2017-04-30 12:24

Using the old system, you need to introduce one new function for any new functionality, that's why I "introduced" it in the first place.

By Maikel Date 2017-04-30 15:15

That how it is done so far, and I don't see a reason to do it differently for one object. So I fixed this mission problem accordingly.

By Sven2 Date 2017-04-30 16:52 Edited 2017-05-01 03:13

Set* functions are the definition of the public interface of the object. It's pretty much standard to user interfaces like this (see e.g. https://en.wikipedia.org/wiki/Mutator_method for a few languages).

It's a standard we use in all objects and the engine (e.g. SetColor(...), not Set("color", ...)). Please don't invent new standards. Even if they make sense on their own, it complicates things to have both variants.

~~A Set function that simply sets a local variable is rather pointless. You could as well have written object.variable = setting; then.~~

By Zapper

Date 2017-04-30 23:17

>A Set function that simply sets a local variable is rather pointless. You could as well have written object.variable = setting; then.

I am not saying we should or should not use that, but there are actually good practical reasons for using a setter for simple variable assignments.
E.g. forward compatibility:
Imagine a library. For the sake of the argument we'll call it Library_Stackable. And this library had to safe whether the contained stack was infinite. In one version it used this.count = -1; in another version it used this.infinite = true;. A setter SetInfinite would have worked in both cases even though it would simply replace a variable assignment in one.

By Sven2

Date 2017-05-01 03:10

> A setter SetInfinite would have worked in both cases even though it would simply replace a variable assignment in one.

Yes, that's why I'm advocating the use of a setter.

But the relaunch rule used Set(setting_name, value), and Set was basically defined as this[setting_name] = value;.

I'm not against having SetX functions for every property X. I'm just against having one generic Set function. We could as well remove all functions from an object and replace it by one big "Do" function. Like some_object->Do("Explode"). But there's no reason to build another layer of abstraction.

You are correct that foo->Set("setting_name", value) is probably better than foo.setting_name = value; because could put compatibility shims into such a generic setter (if (setting_name == "stack_count") do_something_special()). But in the long run, that will be pretty hard to maintain.

By Zapper

Date 2017-05-01 11:51

Oh, yeah. I misunderstood it then. I agree that Set("foo", bar) is not a good idea.

By Isilkor Date 2017-05-01 02:40

> A Set function that simply sets a local variable is rather pointless. You could as well have written object.variable = setting; then.

You could, but please don't. At least with a mutator it's possible to change the internals later.

By Maikel Date 2017-04-25 08:49

Next time you make a change like this and post a pull request you need to do proper testing and put everything into larger commits that work on their own and don't need other commits. Each of the separate commits should work by themselves.

By Fulgen

Date 2017-04-25 13:06

>put everything into larger commits that work on their own and don't need other commits. Each of the separate commits should work by themselves.

Well, in fact, this isn't really easy. E.g., you cannot pick one commit which adapts the scenarios without picking the commit where the new rule was introduced.

By Maikel Date 2017-04-25 13:10

Then both have to go into the same commit, and for sure not into a separate commit for each scenario.

By Fulgen

Date 2017-04-25 14:15

So I should have made a commit for the whole PR?

By Maikel Date 2017-04-25 15:29

I for sure can't make any sense out of all the commits that are flying around between the different PR's.

By Zapper

Date 2017-04-25 18:00

I can tell you about how I plan to do my GUI tinkering: Locally I am working as usual, comitting early and often. Then at some point I decide that I am done. Then I test and fix everything. Before I push that into the official repository, I will whip out git interactive rebase and merge as many commits as possible - maybe even to the point where everything is one commit. No one benefits from seeing my "add GUI bla" and then "fix important bug" as two different commits in that case.

By Fulgen

Date 2017-04-27 05:36

Ok, good point. I'll give this a try, thanks!

By Maikel Date 2017-04-25 09:33 Edited 2017-04-25 13:09

After another set of bugs with parkours. I made these changes to fix them:

* Rule_Restart needs to be replaced by AllowPlayerRestart() (this allows players to restart in the F menu).
* An option to disable handling restarts (so that the Parkour goal can do it properly for Parkours).

I really hope these are the last bugs.

By Fulgen

Date 2017-04-25 13:05

>An option to disable handling restarts (so that the Parkour goal can do it properly for Parkours).

Just return true in OnPlayerRestart(), this aborts the restart.

>Rule_Restart needs to be replaced by AllowPlayerRestart() (this allows players to restart in the F menu).

Don't know exactly what you mean. When restarting is activated, activating Rule_Relaunch in the menu restarts the player.

By Maikel Date 2017-04-25 13:12 Edited 2017-04-25 13:18

Sorry, I was unclear before and edited my post. I made these changes to facilitate Parkour.

>Just return true in OnPlayerRestart(), this aborts the restart.

That is a game call and not to the parkour goal, but I already fixed this.

By Sven2 Date 2017-04-25 14:26

> * An option to disable handling restarts (so that the Parkour goal can do it properly for Parkours).

My plan was that the restart rule also handles restarts in parkour levels. The parkour goal would only provide the restart position (e.g. by hooking into the same GameCallEx that can also be used by scenarios to define respawn positions). That way, we can remove the duplicate restart functionality in the parkour goal and we gain all the extra respawn options that could be used in parkours as well (transfer inventory, restart costs gold, weapon chooser, respawn delay, etc.).

The whole point of having the restart rule was that every scenario and game goal duplicated some of the restart code.

By Maikel Date 2017-04-25 15:31

Well, that plan failed (no offense), and I fixed the resulting bugs. At this point in time I couldn't care less what is intended (again no offense), I just want a working master branch. Feel free to fix it properly (with properly taken as strictly as possible).

By Maikel Date 2017-05-01 10:50

I cleaned up the script a bit, especially documented all setter functions:

This rule enables and handles relaunches with its various aspects. These are:
   * SetInventoryTransfer(bool transfer): inventory of crew is transferred on respawn [default false].
   * SetFreeCrew(bool free): whether the crew is free or needs to be bought [default false].
   * SetBaseRespawn(bool set): whether to respawn at a nearby base [default false].
   * SetLastClonkRespawn(bool b): whether to respawn the last clonk only [default false].
   * SetRespawnDelay(int delay): respawn delay in seconds [default 10].
   * SetAllowPlayerRestart(bool on): whether a player can select restart in the rule menu.
   * SetPerformRestart(bool on): whether this rule actually handles the respawn [default true].
   * SetDefaultRelaunchCount(int r): the number of relaunches a player has [default nil == infinte].
   * SetInitialRelaunch(bool on): whether a relaunch on round start is done [default true].
The active relaunch rule can be obtained by the global function GetRelaunchRule(). The rule also
keeps track of player's actual number of relaunches which can be modified and accessed by:
   * SetPlayerRelaunchCount(int plr, int value): set player relaunch count.
   * GetPlayerRelaunchCount(int plr): get player relaunch count.
   * DoPlayerRelaunchCount(int plr, int value): add to player relaunch count.
   * HasUnlimitedRelaunches(): whether the players have infinite relaunches.

Still todo is proper scenario saving, but for now there is no round yet which places this object anywhere, so that is fine.

By Clonkonaut [de]

Date 2017-05-02 12:48

Just tested through the newest snapshot:

- Dark Castle, Raid, Deep Sea Mining, Treasure Hunt: respawn time is 35 seconds
- Crash Landing: Respawn time is instant, but respawn position makes you stuck
- Shiver Peak: relaunch rule isn't configured thus broken (why is the default respawn time 360 seconds?!)

By Maikel Date 2017-05-02 13:51

Fixed, thanks for spotting these. Please note that only the first was my responsibility!

By Clonkonaut [de]

Date 2017-05-02 13:55

Didn't want to blame you but you've seemed to be the person knowing how to fix these bugs quick and easy! ;)

By jok Date 2017-05-14 22:53

Relaunches in Hot Ice, etc still don't work.

By Luchs Date 2017-05-15 08:42

I looked into this shortly, but couldn't figure out what's wrong. The (unchanged) HotIce script still appears to work fine and creates a new crew member as well as a relaunch container on death, however the player sometimes is eliminated.

By Maikel Date 2017-05-15 10:43 Edited 2017-05-15 11:18

I'll have another look at it, but won't promise anything.

Edit: Might be fixed, ideally tested in a multiplayer online round.

By Armin

Date 2017-06-24 09:49

> * SetAllowPlayerRestart(bool on): whether a player can select restart in the rule menu.

Is this intended as a new symbol besides the respawn rule to click on?

By Maikel Date 2017-06-24 10:10

Yes, this enables restarting in the player menu, otherwise a click should do nothing.

By Maikel Date 2018-03-15 19:53

So we kept Rule_BaseRespawn for backwards compatibility, but this object causes some inconsistency in editor mode and is obsolete, so I would like to remove it in 9.0 because I also think not many external packs use this object and it is easy to change to the new relaunch rule.

Any objections?

By Caesar Date 2018-03-15 23:56

Hm, can we deprecate it in 8.1?

Topic Development / Scenario & Object Development / New relaunch rule

1 2

Post