Skip to content

Trim empty space around edges of Shp(TD) frames#21368

Merged
abcdefg30 merged 1 commit into
OpenRA:bleedfrom
PunkPun:trim-frames
Jul 30, 2024
Merged

Trim empty space around edges of Shp(TD) frames#21368
abcdefg30 merged 1 commit into
OpenRA:bleedfrom
PunkPun:trim-frames

Conversation

@PunkPun

@PunkPun PunkPun commented Mar 12, 2024

Copy link
Copy Markdown
Member

Revives #15148. It appears the original issues were fixed along the years. I suspect the addition of anti-aliasing had an effect on it.

#15184 (comment) is still present but we can't do anything about it. The 2nd option was chosen back then and we can choose it again. Auto bounds is correct much more often than its not anyhow.

@PunkPun

PunkPun commented Mar 12, 2024

Copy link
Copy Markdown
Member Author

The number of sheets generated by --dump-sequence-sheets go down by

TD 2 -> 1
RA 2 -> 1

@RoosterDragon RoosterDragon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we get a list of items where the selection bounds have changed? Could be useful for any follow-up that wants to tidy those up.

Comment thread OpenRA.Mods.Cnc/SpriteLoaders/ShpTDLoader.cs
@PunkPun

PunkPun commented Mar 12, 2024

Copy link
Copy Markdown
Member Author

This is the diff e4be18d

Generated with 69fc1e1

@pchote

pchote commented Jul 26, 2024

Copy link
Copy Markdown
Member

#15194 was implemented as a much more robust solution to batch breakage - meaning that packing everything into one texture was no longer needed.

Do we have any perf numbers to indicate that trimming is worthwhile under this newer system?

@PunkPun

PunkPun commented Jul 26, 2024

Copy link
Copy Markdown
Member Author

as the sprite sheets are uncompressed, the empty space is eating up quite a bit of RAM. So removing empty sheets definitely way more impact than all the micro-optimisations we do

@pchote

pchote commented Jul 26, 2024

Copy link
Copy Markdown
Member

Only 16MB at the default sheet size, so i'm unconvinced that its worth the potential disruption (and that has definitely been true for most of those other optimisations...)

@RoosterDragon

Copy link
Copy Markdown
Member

Yes the focus now would just be on the memory and not the batching gains. <100 lines to reduce our memory footprint by 5% is a nice win in my opinion. There was some pain when it was added originally, but since we've already dealt with the pain of finding and fixing the regressions, we've already paid those costs.

Co-Authored-By: Paul Chote <pchote@users.noreply.github.com>

@abcdefg30 abcdefg30 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code changes lgtm. It seems like this is improving the bounds compared to bleed, e.g. with the badger or civilian houses.
Bleed:
grafik
This PR:
grafik

Although the latter could still be improved, I didn't spot anything bad on a quick check in RA.

@abcdefg30 abcdefg30 merged commit 7b9a173 into OpenRA:bleed Jul 30, 2024
@abcdefg30

Copy link
Copy Markdown
Member

Changelog

@PunkPun PunkPun deleted the trim-frames branch August 3, 2024 08:36
MustaphaTR added a commit to MustaphaTR/Romanovs-Vengeance that referenced this pull request Aug 8, 2024
This was referenced Mar 7, 2025
@PunkPun PunkPun mentioned this pull request Oct 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants