map: Support tilemap layer versions 3+, improve error handling #26
Conversation
This makes the behavior consistent with DDNet and fixes parsing for some unusual maps (fixes heinrich5991#16).
|
I've tested |
| } | ||
|
|
||
| let v0 = format::MapItemLayerV1CommonV0::mandatory(raw, TooShort, InvalidVersion)?; | ||
| let v2 = format::MapItemLayerV1TilemapV2::mandatory(raw, TooShortV2, InvalidVersion)?; | ||
| let v3 = format::MapItemLayerV1TilemapV3::optional(raw, TooShortV3)?; | ||
| let flags = v2.flags as u32; |
There was a problem hiding this comment.
v2.flags are i32, InvalidFlags() value is i32, but TILELAYERFLAG_* are u32.
I've decided to change a few functions to accept flags as i32 to avoid converting i32 -> u32 -> i32. This has removed the need for a separate u32 variable.
There was a problem hiding this comment.
The idea was that unsigned integers behave more like bit containers than signed integers (see arithmetic right shift of signed integers). The conversions and separate variables only exist in source code, to the CPU and the compiler, they live in the same location.
I'm fine with the change.
| .map_err(|e| match e { | ||
| TooShort(_) => too_short(raw.len()), | ||
| other_err => other_err, |
There was a problem hiding this comment.
I'm not sure if we still need different TooShort errors for different types of layers. As of now errors already contain the layer number: Error(Map(Layer(31, Tilemap(TooShortDdraceSwitch(19))))).
| } else if version == 2 { | ||
| MapItemLayerV1TilemapV2::sum_len() | ||
| } else { | ||
| MapItemLayerV1TilemapV3::sum_len() |
There was a problem hiding this comment.
I must say that I'm not a fan of lax parsing, trying to recover from bad values we don't know.
On the one hand, if someone actually increased the tilemap versions, introducing new fields, this could would break in inexplicable ways.
On the other hand, one could argue that no one could do this because it would break the DDNet parsing code. What do you think, would it be easier to change DDNet code?
Other possibilities include: Emit some kind of warning on tilemap version >= 2 (hard to do, we don't have a warning infrastructure in place for that); introduce some kind of lax parsing mode that just copies whatever DDNet is doing (also annoying as it's one more parameter to pass around).
There was a problem hiding this comment.
I could find only one map affected out of DDNet's (def-'s) maps7 collection. That's the map that is mentioned in the issue.
I also can't open it in DDNet (or latest Teeworlds) editor. I think we could keep the parsing logic as is, declare this map corrupted and settle for improved error reporting (I'll submit a separate pull request).
There was a problem hiding this comment.
It's also possible that it's valid for some modded server & editor - but I'm not sure if we want to try to find which mod that was and add support for it.
|
Thanks for being here and improving the library. :) May I ask you whether you're planning to use this for something or are just generally interested in it? |
|
I'm just playing around with it - it's interesting to see Teeworlds bits implemented in Rust. |
This makes the behavior consistent with DDNet and fixes parsing for some unusual maps (fixes #15).