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

Add FilenamePattern support to sequences. #21136

Merged
merged 1 commit into from Nov 16, 2023

Conversation

pchote
Copy link
Member

@pchote pchote commented Oct 19, 2023

Backported from TDHD, this gives modders/mappers a simpler solution for multi-frame pngs.

@penev92
Copy link
Member

penev92 commented Oct 20, 2023

Thank you for your contribution!
This doesn't require the "embed frame metadata into the PNGs" step, right? And it will also work with all the PNGs being inside an apctur.zip?
I just don't like the name PngSheet now, because it no longer loads a sheet 😕

@pchote
Copy link
Member Author

pchote commented Oct 20, 2023

Note that this feature is dealing with filenames before loading the sprite. It doesn't know or care anything about the format of the sprites to be loaded.

The testcase could equally well have been loading a set of shp files (if a sprite contains multiple frames it will use the first).

And it will also work with all the PNGs being inside an apctur.zip?

No, this would need to be implemented as its own sprite format rather than as a sequence key, unless the apctur.zip was explicitly mounted in mod.yaml

Copy link
Member

@RoosterDragon RoosterDragon left a comment

Choose a reason for hiding this comment

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

One fixup, otherwise LGTM.

@pchote
Copy link
Member Author

pchote commented Nov 5, 2023

Fixed, rebased, and dropped testcase (after verifying the fixes do work).

var filename = LoadField(Filename, data, defaults, out var location);

var loadFrames = CalculateFrameIndices(start, length, stride ?? length ?? 0, facings, frames, transpose, reverseFacings, shadowStart);
yield return new ReservationInfo(filename, loadFrames, frames, location);
return new[] { new ReservationInfo(filename, loadFrames, frames, location) };
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change? I don't think I like it 🧐

Copy link
Member Author

@pchote pchote Nov 16, 2023

Choose a reason for hiding this comment

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

The old code made the function a generator, which wasn't compatible with the new code added above (which would otherwise have to explicitly loop over the results and yield them). The subclasses already returned explicit enumerables so this allows the same code patterns to be used in all three places.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, the code above. Derp. Still not ideal but ok.

@penev92
Copy link
Member

penev92 commented Nov 16, 2023

Sadly this only covers about 1/3 of my concerns from the discussion that spawned the PR 🙁 (but is fine as a standalone PR)
Also the rebase while dropping the test commit makes it almost impossible to find the example usage, so I'll leave this here for the record:
image

@pchote
Copy link
Member Author

pchote commented Nov 16, 2023

Rebased.

@penev92 penev92 merged commit 3b67e42 into OpenRA:bleed Nov 16, 2023
3 checks passed
@penev92
Copy link
Member

penev92 commented Nov 16, 2023

Changelog

@pchote pchote deleted the sequence-patterns branch November 16, 2023 13:28
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.

None yet

3 participants