Neverball / neverball

Tilt the floor to roll a ball through an obstacle course before time runs out.
https://neverball.org
Other
351 stars 80 forks source link

Add movers to SOL file format #206

Open fwp opened 4 years ago

fwp commented 4 years ago

Mover data is a bit of an odd one out. It only exists on the vary level and is not derived from a direct base equivalent. The SOL file format doesn't know about movers. This also means that, unlike path or switch indices, mover indices in replay commands do not reference actual SOL file data but rather a list of movers generated at runtime based on body paths. That, in turn, deviates from the usual pattern of letting mapc figure things out and having Neverball simply load the results.

An approach that's perhaps more elegant would be allowing mapc to create and save base versions of movers, each containing a path index, and linking these movers from the simultaneously created bodies. Bodies would no longer contain path indices, and wouldn't exist in vary versions. Instead, there would be vary instances of movers, copying the (initial) path index from the base mover and adding members for time management.

Overview:

struct b_move
{
    int pi;   /* index of initial path               */
};

struct b_body
{
    int ni;
    int l0;
    int lc;
    int g0;
    int gc;

    int mi;   /* index of first (linear) mover       */
    int mj;   /* index of second (rotational) mover  */
};

struct v_move
{
    int pi;   /* index of current path               */

    float t;  /* time on current path (seconds)      */
    int   tm; /* time on current path (milliseconds) */
};
parasti commented 4 years ago

The only benefit I can really see here is that this would solidify (SOL-idify?) the mover index generation, meaning that replay mover commands would use indices that are in the SOL rather than some very (vary?) uncertain internal state. This is a good thing.

That said, these b_move instances 1) literally just hold an integer, 2) could not be deduplicated by mapc (via some uniq_move) because each mover can/must only ever move one body.

Not sure if those are problems, but they're worth documenting.

fwp commented 4 years ago

That said, these b_move instances 1) literally just hold an integer,

Yes. But because that integer is wrapped in a _bmove instance, the _bbody can now reference that instance and be done with it. No more need for a _vbody, which is good because technically, nothing about the body varies, only the mover does. And at runtime, the mover index stored in the body doesn't just lead to the _bmove holding that one integer but also the _vmove holding the actual live data.

[...] 2) could not be deduplicated by mapc (via some uniq_move) because each mover can/must only ever move one body.

Is that really true? All bodies that have the same target (or target2) could share a single mover. The only reason why that's not currently a good idea is because existing replays implicitly expect bodies to have individual movers.

Basically, the main motivation for me is that you can simply create, say, a _vpath from a _bpath, and a _vswch from a _bswch, but when it comes to bodies and movers, there's special code that needs to be run. I would prefer if that happened at map-compile time. This would strengthen the base representation of the SOL and in turn make the replay mover commands more robust.

parasti commented 4 years ago

[...] 2) could not be deduplicated by mapc (via some uniq_move) because each mover can/must only ever move one body.

Is that really true? All bodies that have the same target (or target2) could share a single mover. The only reason why that's not currently a good idea is because existing replays implicitly expect bodies to have individual movers.

Sure, since all movers attached to the same path proceed in identical manner, only one instance of a mover is really necessary per path.

How would replay compatibility be achieved, though? Seems like there would have to be a way to distinguish pre-this-change replays, so the compatibility logic wouldn't trip up new replays. Maybe, e.g., a new replay command that is literally a copy of CMD_MOVEPATH, but with a new command index? Am I overthinking this?

fwp commented 4 years ago

My original thinking was that, at least for the time being, the movers saved in the SOL should be exactly the same (in terms of their number, order and association with bodies) as the in-memory ones we have now. Existing replays would work as before. The benefit of the change would be the transfer of data and logic that can reside on the base level away from the vary level, simplifying the latter. Optimization wouldn't be a part of it.

If we do want to consider optimizing the mover list, I would advise against adding another mover command. Instead, we could introduce a new CMD_VERSION command that assigns an integer-based version number to the command stream, thus enabling the game client to detect under which conditions a replay was written. This command should be sent as part of the first update, with its absence being interpreted as version 0. The number would be increased whenever there are changes that affect replays.

There's one other issue, though. You can skip the uniq passes if you prefer a shorter compile time to shorter SOLs; the game runs basically the same either way. However, unlike materials or vertices, there would be a difference for movers because commands refer to their indices. It doesn't feel right that replays should depend on whether or not an optional optimization is done. There would probably have to be a guarantee that if a mover for a given path index already exists, it is reused instead of creating a new one.

parasti commented 4 years ago

I wrote the code to see what it looks like. Honestly, it's a hassle. It's the cleaner approach, for sure, but it should have been made when movers were introduced. At this point, though, compatibility code has to be added to keep old replays and SOLs working. So there's mover generation on mapc side and mover generation on Neverball side, when the SOL doesn't have movers ... Potentially, replay-to-SOL mover index translation, when replay comes from old Neverball ... You get the idea. It's a net increase in code size and complexity for Neverball. The only function that is simplified is sol_load_vary itself. Hypothetically, an alternate SOL renderer that doesn't care about old Neverball cruft is simplified.

parasti commented 4 years ago

I'm sure there's something we can take away from this. There's maybe a benefit to introducing a b_move struct at SOL load time? So the mover generation would move down a level, but not to mapc level. Maybe worth a thought.

fwp commented 4 years ago

I wrote the code to see what it looks like. Honestly, it's a hassle. It's the cleaner approach, for sure, but it should have been made when movers were introduced.

I kind of agree. Once compatibility considerations enter the picture, the extra code needed can take the fun out of refactoring. I mean, there are already various bits to support older SOLs, most notably for geoms/offsets and material flags. Still, having to duplicate the mover generation code certainly isn't optimal. Perhaps at some later date, possibly for a new release, we could have a clean slate and require mappers to recompile their add-on levels, but that would still leave the issue of mover indices in replays.

There's maybe a benefit to introducing a b_move struct at SOL load time?

That's what I have done for Nuncabola (not yet published). The data structures are as suggested above, and the mover generation is hidden away in the SOL loader. Everywhere else, the movers are simply available as first-class citizens of the SOL data, in both static and dynamic versions.

As for Neverball, it depends on whether you feel it's a change worth making, given that it wouldn't include the advantages of having the mover indices etched in stone in the SOL files or an optimized mover list.