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

bug(serialization): resetGlobals required to reload a world #73

Closed
douglasduteil opened this issue Feb 1, 2022 · 2 comments
Closed

Comments

@douglasduteil
Copy link

Hi @NateTheGreatt

That bitECS of yours is 🚀 for me.

I'm not sure I'm using it the proper way...
Anyway, I found a freaky behavior in the deserialize process, confusing old and new entities.

bitecs0334-bug

I narrow the mismatch to

const eid = removed.length > 0 ? removed.shift() : globalEntityCursor++

After removing some entites, when reloading previous state, the deserialize function seems to reuse cursors that might be already present in packet to load

const newEid = newEntities.get(eid) || addEntity(world)

That, I think, is giving this weird behavior when instead of loading 2 entries, it's loading the last of them with matching previously removed entities, and it allow "another load" of the same packet with dangling entities I guess (me lost a 🧠 )

bitecs0334-bug3

One word : WHY

I simplify the confusing demo above to an integration test to investigate :

// [...]

  it("should load 2 item after removing them", () => {
    // given
    const { assertItems, assertLength, more, less, save, load } = setup();

    // when
    // We add 2 items
    more();
    more();

    assertItems([
      [1, 100],
      [2, 101]
    ]);
    assertLength(2);

    // when
    // We save the state
    const pack = save();

    // when
    // We remove the 2 items
    less();
    less();

    // NOTE(douglasduteil): reseting the globals seems to hack the test
    // HACK(douglasduteil): use setDefaultSize to trigger a resetGlobals
    setDefaultSize(1e5);

    // when
    // We load the last state
    load(pack);

    // then
    assertItems([
      [0, 100],
      [1, 101]
    ]);
    assertLength(2);
  });


// [...]

I'm not sure about the setDefaultSize above...
It make the test pass but smells pretty wrong, I don't know ... (404 on 🧠 again)

Here is a test case sandbox to illustrate the behavior.
https://codesandbox.io/s/bitecs-0-3-34-serialize-test-b7uxt?file=/Serialize.test.js

Might be related to #67

Thanks for the reading :)
Looking forward to new versions.

🧠 🛹

@EnderShadow8
Copy link
Contributor

EnderShadow8 commented Feb 2, 2022

The serialiser doesn't capture a snapshot of the state at the time, but rather the diff between the current capture and the previous capture. This is useful for syncing state between a server and client without sending large packets containing the entire state.

An improved library with more features including complete snapshots is being developed here: https://github.com/Web-ECS/SoA-serialization

Related: #70

@douglasduteil
Copy link
Author

Ooooh good to know
I'll look it up !
Thanks @EnderShadow8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants