feat: Add on_script_reloaded callback.

[shanecelis]

Summary

I added an on_script_reloaded function that is called before and after on_script_loaded on reloads. This addresses the issue I filed #419.

I updated the core-callbacks doc with example code. Here's the example code shown there:

mode = 1
function on_script_reloaded(save, value)
    if save then
        print("Before I go, take this.")
        return mode
    else
        print("I'm back. Where was I?")
        mode = value
    end
end

I'm not certain about the name. on_script_reload might be the name I'd choose with a blank slate.

Alternatives

One could designate a special variable to capture. But I think a function is preferable because a reload can interrupt at any time and keeping that value up to date all the time instead of when it's required seems inefficient.

One could break this into two functions, but they don't do anything unless both are implemented so it seemed wise to keep them as a single function.

Testing

I exercised it manually with errors in both the saving of state and restoring state, and both were reported as the usual mechanism.

Mistake

I started this branch on top of my other pull request #418. Oops. Luckily #418 is not controversial or large.

[shanecelis]

A further alternative

I guess as an alternative going with the two functions, one could do this:

mode = 1
function on_script_reload()
    print("Before I go, take this.")
    return mode
end

function on_script_loaded(value)
    if value then
        print("I'm back. Where was I?")
        mode = value
    end
end

My criticism above doesn't apply because on_script_loaded() is useful without on_script_reload(). We'd then only have this set of function calls when reloading:

1. on_script_loaded()
2. INITIATE RELOAD
3. value = on_script_reload()
4. CONTEXT CLEARED
5. on_script_loaded(value)

The preceding design looks like this for comparison:

1. on_script_loaded()
2. INITIATE RELOAD
3. value = on_script_reload(true)
4. CONTEXT CLEARED
5. on_script_loaded()
6. on_script_reload(false, value)

[shanecelis]

NOTE: This currently only works when context sharing is enabled. I made an attempt to support non-shared contexts, but it seemed potentially infeasible. I'd love to hear @makspll's take on whether that seems right.

[github-actions[bot]]

Bencher Report

Branch	reload-hook
Testbed	linux-gha

Click to view all benchmark results

Benchmark	Latency	Benchmark Result nanoseconds (ns) (Result Δ%)	Upper Boundary nanoseconds (ns) (Limit %)
component/access Lua	📈 view plot 🚷 view threshold	3,356.40 ns (-29.12%) Baseline: 4,735.63 ns	6,251.86 ns (53.69%)
component/access Rhai	📈 view plot 🚷 view threshold	5,288.60 ns (-20.38%) Baseline: 6,642.14 ns	7,952.68 ns (66.50%)
component/get Lua	📈 view plot 🚷 view threshold	2,078.90 ns (-25.77%) Baseline: 2,800.79 ns	3,522.16 ns (59.02%)
component/get Rhai	📈 view plot 🚷 view threshold	4,076.10 ns (-16.59%) Baseline: 4,886.73 ns	5,922.75 ns (68.82%)
conversions/Mut::from	📈 view plot 🚷 view threshold	81.90 ns (-34.07%) Baseline: 124.23 ns	229.05 ns (35.76%)
conversions/Ref::from	📈 view plot 🚷 view threshold	75.41 ns (-38.23%) Baseline: 122.08 ns	226.79 ns (33.25%)
conversions/ScriptValue::List	📈 view plot 🚷 view threshold	259.65 ns (-51.59%) Baseline: 536.34 ns	1,080.93 ns (24.02%)
conversions/ScriptValue::Map	📈 view plot 🚷 view threshold	1,207.10 ns (-23.47%) Baseline: 1,577.34 ns	2,151.49 ns (56.11%)
conversions/ScriptValue::Reference::from_into	📈 view plot 🚷 view threshold	26.40 ns (-31.74%) Baseline: 38.67 ns	61.37 ns (43.02%)
conversions/Val::from_into	📈 view plot 🚷 view threshold	264.01 ns (-21.26%) Baseline: 335.30 ns	445.67 ns (59.24%)
function/call 4 args Lua	📈 view plot 🚷 view threshold	1,635.10 ns (-20.13%) Baseline: 2,047.09 ns	2,475.44 ns (66.05%)
function/call 4 args Rhai	📈 view plot 🚷 view threshold	1,332.80 ns (-23.90%) Baseline: 1,751.32 ns	2,212.39 ns (60.24%)
function/call Lua	📈 view plot 🚷 view threshold	230.35 ns (-15.90%) Baseline: 273.89 ns	330.70 ns (69.66%)
function/call Rhai	📈 view plot 🚷 view threshold	400.95 ns (-22.11%) Baseline: 514.79 ns	663.70 ns (60.41%)
loading/empty Lua	📈 view plot 🚷 view threshold	55,918.00 ns (-41.84%) Baseline: 96,146.80 ns	134,387.11 ns (41.61%)
loading/empty Rhai	📈 view plot 🚷 view threshold	643,330.00 ns (-49.04%) Baseline: 1,262,463.00 ns	1,850,611.06 ns (34.76%)
math/vec mat ops Lua	📈 view plot 🚷 view threshold	6,773.50 ns (-12.59%) Baseline: 7,749.13 ns	9,030.06 ns (75.01%)
math/vec mat ops Rhai	📈 view plot 🚷 view threshold	6,212.80 ns (-15.30%) Baseline: 7,335.24 ns	8,457.67 ns (73.46%)
query/10 entities Lua	📈 view plot 🚷 view threshold	18,561.00 ns (-22.66%) Baseline: 23,999.16 ns	30,097.45 ns (61.67%)
query/10 entities Rhai	📈 view plot 🚷 view threshold	18,651.00 ns (-22.15%) Baseline: 23,957.55 ns	28,472.01 ns (65.51%)
query/100 entities Lua	📈 view plot 🚷 view threshold	38,798.00 ns (-20.41%) Baseline: 48,746.00 ns	60,127.30 ns (64.53%)
query/100 entities Rhai	📈 view plot 🚷 view threshold	31,741.00 ns (-17.30%) Baseline: 38,382.05 ns	46,900.20 ns (67.68%)
query/1000 entities Lua	📈 view plot 🚷 view threshold	258,370.00 ns (-17.32%) Baseline: 312,477.95 ns	376,275.50 ns (68.67%)
query/1000 entities Rhai	📈 view plot 🚷 view threshold	170,680.00 ns (-19.87%) Baseline: 212,990.91 ns	289,766.71 ns (58.90%)
reflection/10 Lua	📈 view plot 🚷 view threshold	5,496.90 ns (-15.00%) Baseline: 6,467.23 ns	7,451.81 ns (73.77%)
reflection/10 Rhai	📈 view plot 🚷 view threshold	15,216.00 ns (-6.66%) Baseline: 16,301.98 ns	17,844.39 ns (85.27%)
reflection/100 Lua	📈 view plot 🚷 view threshold	47,412.00 ns (-11.04%) Baseline: 53,298.64 ns	59,588.41 ns (79.57%)
reflection/100 Rhai	📈 view plot 🚷 view threshold	708,980.00 ns (-4.98%) Baseline: 746,171.59 ns	874,729.94 ns (81.05%)
resource/access Lua	📈 view plot 🚷 view threshold	3,027.30 ns (-27.62%) Baseline: 4,182.68 ns	5,432.95 ns (55.72%)
resource/access Rhai	📈 view plot 🚷 view threshold	4,638.00 ns (-21.59%) Baseline: 5,914.72 ns	7,330.40 ns (63.27%)
resource/get Lua	📈 view plot 🚷 view threshold	1,699.60 ns (-27.10%) Baseline: 2,331.50 ns	2,968.34 ns (57.26%)
resource/get Rhai	📈 view plot 🚷 view threshold	3,488.00 ns (-17.44%) Baseline: 4,224.93 ns	5,238.47 ns (66.58%)

🐰 View full continuous benchmarking report in Bencher

[makspll]

My thoughts so far:

In favour ✅

• I think the logic is sound, having an explicit callback you have control of as a script writer is nice
  • I suppose we can also go in the direction of having on_script_serialized and on_script_deserialized if we want to have more explicit control from the rust side
• Since working with shared contexts is currently impossible and I am not actively working on them, and with this making them usable, very happy to merge and experiment with this approach

Points of hmm 🤔

• it seems to me this should be possible to make work in non-shared contexts too
  • what were the problems you faced here?
  • having these callbacks work slightly differently between the two modes of bms would be unnecessarily confusing IMO
• (very minor) performance downgrade
  • but ONLY on loads and reloads, which I don't really have a problem with, and in the future we'd need to add more callbacks into this lifecycle anyhow (so optimizing them somehow will be future work)

[shanecelis]

• it seems to me this should be possible to make work in non-shared contexts too

  • what were the problems you faced here?

Let me see if I can recreate the problem I faced. Huh. Well, that's funny. It's working now and it's as easy as I hoped it would be. New PR #424 incoming.

The block of code that calls the on_script_reloaded(true) is virtually identical for both cases and maybe can be extracted into one block. The shared-context case had some conditional code that suggested it made sense to only call it in one of its cases. It works now for both cases, but I'll leave it to your more practiced hands on this code base if the code duplication ought to be refactored.

• having these callbacks work slightly differently between the two modes of bms would be unnecessarily confusing IMO

I agree. It'd be best to unify them.

[shanecelis]

I am sorry. I got confused because you merged main into my branch, which is fine, but I interpreted it as this PR had gotten merged into your repo, which isn't the case. That's why for a follow up I created the #424 PR. Ugh. Sorry about that. Let me try to get back on track with this PR.

[makspll]

I am sorry. I got confused because you merged main into my branch, which is fine, but I interpreted it as this PR had gotten merged into your repo, which isn't the case. That's why for a follow up I created the https://github.com/makspll/bevy_mod_scripting/pull/424 PR. Ugh. Sorry about that. Let me try to get back on track with this PR.

haha no worries!

thoughts on the changes I made?