Skip to content

Issue #234 Paint in Place Removed Image Changes#330

Merged
jrconway3 merged 155 commits intoLiberatedPixelCup:paint-in-place-masterfrom
jrconway3:feature/issue-234-paint-in-place-remove-image-changes
Mar 7, 2026
Merged

Issue #234 Paint in Place Removed Image Changes#330
jrconway3 merged 155 commits intoLiberatedPixelCup:paint-in-place-masterfrom
jrconway3:feature/issue-234-paint-in-place-remove-image-changes

Conversation

@jrconway3
Copy link
Contributor

Removed the spritesheet restructure from #313 so we can easier see the code changes.

There are still some image changes, but only the ones necessary to make skintones work for all assets (namely this still adds missing base assets for Wolf/Cat Ear Skintones and Wrinkles).

JaidynReiman added 30 commits January 12, 2026 01:43
…ons & Materials In Metadata Instead of Constants
@jrconway3
Copy link
Contributor Author

Thanks for the additional comments. I'll review them shortly.

@jrconway3
Copy link
Contributor Author

#330 (comment)

For some stupid reason I cannot leave a response comment or mark this as resolved anymore when it was literally just working an hour ago.

Anyway, I looked into this and I watched the video. I still have no idea what the issue is, at no point did I find it being slow at all. I asked Copilot to find a resolution, I tested it, it works but its not any faster or slower than before, so frankly, I don't really see what the problem is.

I guess the argument is that if there's enough entries it COULD cause massive slowdown, but frankly the odds of that happening in the future are very slim because not many assets will have extra colors anyway. But feel free to look at this solution and let me know how it looks.

@jrconway3 jrconway3 requested review from Gaurav0 and Copilot March 5, 2026 02:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 139 out of 413 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Gaurav0
Copy link
Member

Gaurav0 commented Mar 5, 2026

#330 (comment)

For some stupid reason I cannot leave a response comment or mark this as resolved anymore when it was literally just working an hour ago.

Anyway, I looked into this and I watched the video. I still have no idea what the issue is, at no point did I find it being slow at all. I asked Copilot to find a resolution, I tested it, it works but its not any faster or slower than before, so frankly, I don't really see what the problem is.

I guess the argument is that if there's enough entries it COULD cause massive slowdown, but frankly the odds of that happening in the future are very slim because not many assets will have extra colors anyway. But feel free to look at this solution and let me know how it looks.

Well this certainly should be much faster just by looking at the new code, but without instrumenting it would be hard to tell if it actually is. Interesting that its using \u0000 as a delimiter in the key.

There is already code in the repo demonstrating how to instrument performance. See

// Mark end and measure
if (profiler) {
profiler.mark("renderCharacter:end");
profiler.measure(
"renderCharacter",
"renderCharacter:start",
"renderCharacter:end"

@jrconway3
Copy link
Contributor Author

#330 (comment)
For some stupid reason I cannot leave a response comment or mark this as resolved anymore when it was literally just working an hour ago.
Anyway, I looked into this and I watched the video. I still have no idea what the issue is, at no point did I find it being slow at all. I asked Copilot to find a resolution, I tested it, it works but its not any faster or slower than before, so frankly, I don't really see what the problem is.
I guess the argument is that if there's enough entries it COULD cause massive slowdown, but frankly the odds of that happening in the future are very slim because not many assets will have extra colors anyway. But feel free to look at this solution and let me know how it looks.

Well this certainly should be much faster just by looking at the new code, but without instrumenting it would be hard to tell if it actually is. Interesting that its using \u0000 as a delimiter in the key.

There is already code in the repo demonstrating how to instrument performance. See

// Mark end and measure
if (profiler) {
profiler.mark("renderCharacter:end");
profiler.measure(
"renderCharacter",
"renderCharacter:start",
"renderCharacter:end"

I added a profiler, let me know how that looks. Its not reporting anything, but I checked and confirmed that it is hitting the profiler by throwing console logs into the if (profiler) { blocks.

@Gaurav0
Copy link
Member

Gaurav0 commented Mar 7, 2026

I added a profiler, let me know how that looks. Its not reporting anything, but I checked and confirmed that it is hitting the profiler by throwing console logs into the if (profiler) { blocks.

You just had to change verbose flag to true to see the report. We might want to put another querystring flag for this or default it to true for DEBUG.

While the new code is now more than 100x faster, ~150ms vs ~1.3ms, you don't notice because loading images and rerendering the character takes much longer. May as well keep the faster code though.

@jrconway3 jrconway3 merged commit 3944ce8 into LiberatedPixelCup:paint-in-place-master Mar 7, 2026
7 checks passed
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.

3 participants