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.

Sign up for GitHub

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

map: Support tilemap layer versions 3+, improve error handling #26

Closed
wants to merge 1 commit into from
Closed

map: Support tilemap layer versions 3+, improve error handling #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions map/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,31 +239,33 @@ pub struct MapItemLayerV1TilemapExtraRace {
pub data: i32,
}
impl MapItemLayerV1TilemapExtraRace {
pub fn offset(version: i32, flags: u32) -> Option<usize> {
let offset = match version {
2 => MapItemLayerV1TilemapV2::sum_len(),
3 => MapItemLayerV1TilemapV3::sum_len(),
_ => return None,
pub fn offset(version: i32, flags: i32)
-> Result<usize, LayerTilemapError>
{
let offset = if version < 2 {
return Err(LayerTilemapError::InvalidVersion(version))
} else if version == 2 {
MapItemLayerV1TilemapV2::sum_len()
} else {
MapItemLayerV1TilemapV3::sum_len()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@Fireball-Teeworlds Fireball-Teeworlds Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
Some(offset + match flags {
Ok(offset + match flags as u32 {
TILELAYERFLAG_TELEPORT => 0,
TILELAYERFLAG_SPEEDUP => 1,
TILELAYERFLAG_FRONT => 2,
TILELAYERFLAG_SWITCH => 3,
TILELAYERFLAG_TUNE => 4,
_ => return None,
_ => return Err(LayerTilemapError::InvalidFlags(flags)),
})
}
pub fn from_slice(slice: &[i32], version: i32, flags: u32)
-> Option<&MapItemLayerV1TilemapExtraRace>
pub fn from_slice(slice: &[i32], version: i32, flags: i32)
-> Result<&MapItemLayerV1TilemapExtraRace, LayerTilemapError>
{
let offset = unwrap_or_return!(
MapItemLayerV1TilemapExtraRace::offset(version, flags), None
);
let offset = MapItemLayerV1TilemapExtraRace::offset(version, flags)?;
if slice.len() <= offset {
return None;
return Err(LayerTilemapError::TooShort(slice.len()));
}
Some(&(unsafe { common::slice::transmute(slice) })[offset])
Ok(&(unsafe { common::slice::transmute(slice) })[offset])
}
}

Expand Down
30 changes: 16 additions & 14 deletions map/src/generate_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,31 +291,33 @@
pub data: i32,
}
impl MapItemLayerV1TilemapExtraRace {
pub fn offset(version: i32, flags: u32) -> Option<usize> {
let offset = match version {
2 => MapItemLayerV1TilemapV2::sum_len(),
3 => MapItemLayerV1TilemapV3::sum_len(),
_ => return None,
pub fn offset(version: i32, flags: i32)
-> Result<usize, LayerTilemapError>
{
let offset = if version < 2 {
return Err(LayerTilemapError::InvalidVersion(version))
} else if version == 2 {
MapItemLayerV1TilemapV2::sum_len()
} else {
MapItemLayerV1TilemapV3::sum_len()
};
Some(offset + match flags {
Ok(offset + match flags as u32 {
TILELAYERFLAG_TELEPORT => 0,
TILELAYERFLAG_SPEEDUP => 1,
TILELAYERFLAG_FRONT => 2,
TILELAYERFLAG_SWITCH => 3,
TILELAYERFLAG_TUNE => 4,
_ => return None,
_ => return Err(LayerTilemapError::InvalidFlags(flags)),
})
}
pub fn from_slice(slice: &[i32], version: i32, flags: u32)
-> Option<&MapItemLayerV1TilemapExtraRace>
pub fn from_slice(slice: &[i32], version: i32, flags: i32)
-> Result<&MapItemLayerV1TilemapExtraRace, LayerTilemapError>
{
let offset = unwrap_or_return!(
MapItemLayerV1TilemapExtraRace::offset(version, flags), None
);
let offset = MapItemLayerV1TilemapExtraRace::offset(version, flags)?;
if slice.len() <= offset {
return None;
return Err(LayerTilemapError::TooShort(slice.len()));
}
Some(&(unsafe { common::slice::transmute(slice) })[offset])
Ok(&(unsafe { common::slice::transmute(slice) })[offset])
}
}

Expand Down
20 changes: 11 additions & 9 deletions map/src/reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,18 +331,20 @@ impl LayerTilemap {
use format::ColorComponent::*;
use format::LayerTilemapError::*;

fn extra<TS>(raw: &[i32], version: i32, flags: u32, too_short: TS)
fn extra<TS>(raw: &[i32], version: i32, flags: i32, too_short: TS)
-> Result<&format::MapItemLayerV1TilemapExtraRace, format::LayerTilemapError>
where TS: FnOnce(usize) -> format::LayerTilemapError
{
format::MapItemLayerV1TilemapExtraRace::from_slice(raw, version, flags)
.ok_or_else(|| too_short(raw.len()))
.map_err(|e| match e {
TooShort(_) => too_short(raw.len()),
other_err => other_err,
Comment on lines +339 to +341
Copy link
Contributor Author

@Fireball-Teeworlds Fireball-Teeworlds Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))))).

})
}

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// TODO: Standard settings for game group.
let color = Color {
Expand All @@ -364,7 +366,7 @@ impl LayerTilemap {
let image = get_index_opt(v2.image, image_indices, InvalidImageIndex)?;
let data = get_index(v2.data, data_indices.clone(), InvalidDataIndex)?;
let mut normal = false;
let type_ = match flags {
let type_ = match v2.flags as u32 {
0 => {
normal = true;
LayerTilemapType::Normal(LayerTilemapNormal {
Expand All @@ -380,7 +382,7 @@ impl LayerTilemap {
format::TILELAYERFLAG_TELEPORT => {
LayerTilemapType::RaceTeleport(
get_index(
extra(raw, v0.version, flags, TooShortRaceTeleport)?.data,
extra(raw, v0.version, v2.flags, TooShortRaceTeleport)?.data,
data_indices.clone(),
InvalidRaceTeleportDataIndex,
)?,
Expand All @@ -390,7 +392,7 @@ impl LayerTilemap {
format::TILELAYERFLAG_SPEEDUP => {
LayerTilemapType::RaceSpeedup(
get_index(
extra(raw, v0.version, flags, TooShortRaceSpeedup)?.data,
extra(raw, v0.version, v2.flags, TooShortRaceSpeedup)?.data,
data_indices.clone(),
InvalidRaceSpeedupDataIndex,
)?,
Expand All @@ -400,7 +402,7 @@ impl LayerTilemap {
format::TILELAYERFLAG_FRONT => {
LayerTilemapType::DdraceFront(
get_index(
extra(raw, v0.version, flags, TooShortDdraceFront)?.data,
extra(raw, v0.version, v2.flags, TooShortDdraceFront)?.data,
data_indices.clone(),
InvalidDdraceFrontDataIndex,
)?,
Expand All @@ -410,7 +412,7 @@ impl LayerTilemap {
format::TILELAYERFLAG_SWITCH => {
LayerTilemapType::DdraceSwitch(
get_index(
extra(raw, v0.version, flags, TooShortDdraceSwitch)?.data,
extra(raw, v0.version, v2.flags, TooShortDdraceSwitch)?.data,
data_indices.clone(),
InvalidDdraceSwitchDataIndex,
)?,
Expand All @@ -420,7 +422,7 @@ impl LayerTilemap {
format::TILELAYERFLAG_TUNE => {
LayerTilemapType::DdraceTune(
get_index(
extra(raw, v0.version, flags, TooShortDdraceTune)?.data,
extra(raw, v0.version, v2.flags, TooShortDdraceTune)?.data,
data_indices.clone(),
InvalidDdraceTuneDataIndex,
)?,
Expand Down