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

Compress TileMap tiles in level file #3124

Merged
merged 11 commits into from
Dec 5, 2024
Merged

Conversation

Vankata453
Copy link
Member

@Vankata453 Vankata453 commented Dec 2, 2024

TileMap tiles are now saved with the help of negative integers, the absolute value of each one being a multiplier for the next value (ex. "0 0 0 0 0 5 5 5" -> "-5 0 -3 5").

Newline separation for tiles in file based on TileMap width is no longer supported to save off additional file size.

Examples:

  • 0 0 0 0 0 0 45 1 1 1 1 3 3 3 3 3 0 0 -> -6 0 45 -4 1 -5 3 -2 0
  • 0 0 0 0 0 43 12 12 12 12 12 1 1 1 5 -> -5 0 43 -5 12 -3 1 5

Results from re-saving some levels from "The Crystal Catacombs":

  • Worldmap: 1.5MiB -> 74.9KiB
  • "The Journey Begins Again": 409.2KiB -> 27.7KiB
  • "Fjord of Fortitude": 319.1KiB -> 48.9KiB
  • "A Light in the Darkness": 613.3KiB -> 183.9KiB
  • "Light and Magic": 456KiB -> 163.0KiB

`TileMap` tiles are now saved by replacing a sequence of empty tiles (ex. "0 0 0 0 0") with a single negative number, the absolute value of which preserves the amount of sequential empty tiles (ex. "-5").

Examples:

"0 0 0 0 0 0 45 3 0 0" -> "-6 45 3 -2"
"0 0 0 0 0 43 0 0 0 0 12 0 0 0" -> "-5 43 -4 12 -3"

Results from re-saving some levels from "The Crystal Catacombs":

"The Journey Begins Again": 409.2KiB -> 40KiB
"Fjord of Fortitude": 319.1KiB -> 65.4KiB
"A Light in the Darkness": 613.3KiB -> 301.5KiB
"Light and Magic": 456KiB -> 173KiB
@Vankata453 Vankata453 added involves:performance category:code status:needs-review Work needs to be reviewed by other people labels Dec 2, 2024
Copy link
Member

@weluvgoatz weluvgoatz left a comment

Choose a reason for hiding this comment

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

Tested the first 4 levels of #3121:
all_that_glistens.stl: 437KB -> 194KB
another_cold_day.stl: 918KB -> 167KB
area_42.stl: 838KB -> 454KB
bonus_dias.stl: 337KB -> 107KB
And they all seem to work just fine!

@swagtoy
Copy link
Contributor

swagtoy commented Dec 4, 2024

@Vankata453 Is there a possibility you could implement a unit test for this?

`TileMap` tiles are now saved with the help of negative integers, the absolute value of each one being a multiplier for the next value (ex. "0 0 0 0 0 5 5 5" -> "-5 0 -3 5").

Newline separation for tiles in file based on `TileMap` width is still supported.

Examples:

* `0 0 0 0 0 0 45 1 1 1 1 3 3 3 3 3 0 0` -> `-6 0 45 -4 1 -5 3 -2 0`
* `0 0 0 0 0 43 12 12 12 12 12 1 1 1 5` -> `-5 0 43 -5 12 -3 1 5`

Results from re-saving some levels from "The Crystal Catacombs":

* **Worldmap**: 1.5MiB -> 131.5KiB
* "The Journey Begins Again": 409.2KiB -> 35.8KiB
* "Fjord of Fortitude": 319.1KiB -> 53.1KiB
* "A Light in the Darkness": 613.3KiB -> 190.2KiB
* "Light and Magic": 456KiB -> 168.9KiB
@Vankata453 Vankata453 requested a review from weluvgoatz December 4, 2024 22:07
@swagtoy
Copy link
Contributor

swagtoy commented Dec 4, 2024

Can you try breaking your test now? Primarily, we already know it works, but try breaking it, Try reading nothing at the end of the tiles and seeing if it breaks. Assume the level designer would maliciously put a negative at the end to break stuff and see if an exception throws or something.

@Vankata453
Copy link
Member Author

I already ensured it throws exception if two negative integers are next to each other and if the array ends with a negative value.

src/util/reader_mapping.cpp Outdated Show resolved Hide resolved
src/util/reader_mapping.cpp Show resolved Hide resolved
src/util/writer.cpp Outdated Show resolved Hide resolved
src/util/writer.cpp Outdated Show resolved Hide resolved
tests/reader_test.cpp Show resolved Hide resolved
@swagtoy
Copy link
Contributor

swagtoy commented Dec 4, 2024

Oh my apologies, i only read the first test function and missed that (i skimmed that as you "adding the test cases"). Moving on then. If the logic works then it looks good :) Sent some suggestions I think

@swagtoy
Copy link
Contributor

swagtoy commented Dec 5, 2024

LGTM. It look great son lets merge it

@swagtoy
Copy link
Contributor

swagtoy commented Dec 5, 2024

Actually, wait no, I do think we could maybe throw a dart and maybe align on a single Vector::reserve() size, just so for example all those 32 4 5 5 5 5 5 5 5 5 5 42 5 cases where a tile repeats maybe 10 or 15 times to create a wall won't result in needless reallocations. We could even do step scaling (i.e. for a step of 5, if 6 elements are pushed, then it allocates to 10)... EFL can do this but I see nothing in C++ that can easily let us do a step. We'd have to implement that logic ourselves, though I actually question if that'd help. At this point we would be micro-optimizing to very OS dependent tasks. So at a minimum, lets just reserve and clear for now on a vector so we can very slightly speed up those few cases. Maybe a value of 24 seems generous. Up to you here; once you decide on that you can then merge.

@swagtoy
Copy link
Contributor

swagtoy commented Dec 5, 2024

lgtm

Copy link
Contributor

@swagtoy swagtoy left a comment

Choose a reason for hiding this comment

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

heh

@Vankata453 Vankata453 merged commit 74ada4f into master Dec 5, 2024
28 of 33 checks passed
@Vankata453 Vankata453 deleted the compress-tilemap-tiles branch December 5, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:code involves:performance status:needs-review Work needs to be reviewed by other people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants