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

Flatten tiles to vec u32 #58

Closed

Conversation

satrix321
Copy link

Hi,
This change flattens the tiles from a layer into a structure containing Vec (instead of Vec<Vec>) for performance concerns. There's also a method provided for easy conversion to Vec<Vec> if there would be need for it.

As of now this is a breaking change, but with some minor adjustments users would still be able to iterate over this collection by rows (by using tiles_per_row and number_of_rows). I'm open to any suggestions how we could smooth this out a bit more.

* Create TilesContainer for ease of use
* Provide some means to easily convert to Vec Vec u32
* Override [], use as [row][column]
* added number_of_rows field
@mattyhall
Copy link
Contributor

Sorry for the delay, I'll try and look at this at the weekend!

@mattyhall
Copy link
Contributor

Sorry for the delay again. Life got in the way I'm afraid :(

It looks good. I'm not too bothered about the breaking version as we're still <1.0 (and I doubt anyone is seriously using the library) but maybe we could add a conversion from TilesContainer to Vec<Vec<_>>

@satrix321
Copy link
Author

satrix321 commented Oct 18, 2019

Hey, I've tried looking into this conversion, and it seems to me that we could add a generic one for Vec<Vec>, but it will require a wrapper struct to work properly. Something along those lines:

struct TilesContainerWrapper<T>(Vec<Vec<T>>);
impl<T> From<&TilesContainer> for TilesContainerWrapper<T> where T: std::convert::TryFrom<u32> {
    fn from(item: &TilesContainer) -> TilesContainerWrapper<T> {
        let mut data: Vec<Vec<T>> = Vec::new();
        let tiles_per_row = item.tiles_per_row as usize;
        let number_of_rows = item.number_of_rows as usize;
        for x in 0..number_of_rows {
            let start_index = x * tiles_per_row;
            let end_index = start_index + tiles_per_row;
            let row: Vec<T> = (&item.data[start_index..end_index])
                .iter()
                .cloned()
                .map(|e| T::try_from(e).ok().unwrap())
                .collect();
            data.push(row);
        }
        TilesContainerWrapper(data)
    }
}

I'm not exactly sure if this is something desirable in this case. We could just go with implementing the conversion manually for all built in numeric types, but it would produce a lot of boilerplate code. I'm open to any suggestions on how we could improve this solution.

@bjorn bjorn linked an issue Dec 23, 2021 that may be closed by this pull request
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

@satrix321 This seems like a good change to make. Would you still be able to rebase this change to resolve the conflicts? I've also left some comments.

pub struct TilesContainer {
pub data: Vec<u32>,
pub tiles_per_row: u32,
pub number_of_rows: u32,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just width and height would do as property names?

pub number_of_rows: u32,
}

impl From<&TilesContainer> for Vec<Vec<u32>> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this converter. What use would people have for converting to this sub-optimal data structure? Maybe instead it could be useful to provide a getter to retrieve a tile given x and y coordinates?

@@ -653,7 +678,11 @@ impl Layer {
],
TiledError::MalformedAttributes("layer must have a name".to_string())
);
let mut tiles = Vec::new();
let mut tiles = TilesContainer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be initialized with correct data from the start? I feel like this could lead to invalid states where the tiles member never got initialized.

}
return Ok(rows);
let data: Vec<u32> = s
.replace('\r', "")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think trim after split would work here.

src/lib.rs Show resolved Hide resolved
@aleokdev aleokdev added this to the 0.10.0 milestone Dec 27, 2021
@bjorn
Copy link
Member

bjorn commented Jan 24, 2022

Superseded by #128.

@bjorn bjorn closed this Jan 24, 2022
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

Successfully merging this pull request may close these issues.

Vec<Vec<T>> is inefficient
4 participants