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

Fix: Extract functions no longer throw an error if the frame is empty #9363

Merged
merged 8 commits into from
May 24, 2023

Conversation

dev7355608
Copy link
Collaborator

@dev7355608 dev7355608 commented Apr 10, 2023

The alternative is that we don't allow empty frames and throw an error ourselves if frame is empty that is perhaps more more helpful can the error messages generated by getImageData etc.

Pre-Merge Checklist
  • Tests and/or benchmarks are included
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 10, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5691829:

Sandbox Source
pixijs/pixijs Configuration

@@ -234,8 +234,8 @@ export class CanvasExtract implements ISystem, IExtract

const x = Math.round(frame.x * resolution);
const y = Math.round(frame.y * resolution);
const width = Math.round(frame.width * resolution);
const height = Math.round(frame.height * resolution);
const width = Math.max(Math.round(frame.width * resolution), 1);
Copy link
Member

Choose a reason for hiding this comment

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

i like this! perhaps we set to 1, but also log a warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure. If the render target resolution is low enough, something like extract.pixels(rt, new Rectangle(x, y, 1, 1)) to pick the the color of the pixel at (x, y) might spam an warning unnecessarily. At resolution above 1 if might return more than the 1 pixel we need. I was thinking maybe extract.pixels(rt, new Rectangle(x, y, 0, 0)) could be useful to just extract the one pixel at that point, but the rounding behavior doesn't work for that. Maybe we should use frame.ceil().

@bigtimebuddy
Copy link
Member

What would be better is if we can assert the input before doing anything and throw a specific user-friendly error instead of defaulting to 1x1.

@dev7355608
Copy link
Collaborator Author

What would be better is if we can assert the input before doing anything and throw a specific user-friendly error instead of defaulting to 1x1.

Should we switch to frame.ceil() rounding though? I think it's more natural and cannot throw an error if extract.pixels(rt, new Rectangle(x, y, 1, 1)) is attempted on a low resolution texture.

@bigtimebuddy
Copy link
Member

I think ceil would make sense, yes.

@dev7355608
Copy link
Collaborator Author

Maybe defaulting to 1x1 and no error is better after all considering generateTexture does the same if the extracted region is empty.

@dev7355608
Copy link
Collaborator Author

Reverted the throwing of an error: If generateTexture doesn't throw if empty, then it should be acceptable for the extraction methods to behave the same way.

Reverted the ceil rounding: If the extraction method ceil the frame, then generateTexture should probably ceil the width and height of the region too. Ceiling probably makes more sense in both cases, but I'll leave that for another PR.

@bigtimebuddy
Copy link
Member

This has deviated a little from the original title of the PR. How would you rename this now?

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label May 18, 2023
@bigtimebuddy bigtimebuddy added this to the v7.3.0 milestone May 18, 2023
@dev7355608
Copy link
Collaborator Author

Technically one of the others PR merged before this one already prevented the errors, but didn't mention it. Not sure how to describe the change in a single sentence:

CanvasExtract/Extract functions no longer throw an error if the frame is empty. If the width/height of the frame is less than 1 pixel, it is rounded up to 1 pixel just like the width/height of region parameter of generateTexture is.

@bigtimebuddy bigtimebuddy changed the title Fix: Extraction throws error if frame is empty Fix: Extract functions no longer throw an error if the frame is empty May 18, 2023
@bigtimebuddy
Copy link
Member

@dev7355608 could you resolve conflicts so I could merge this?

@bigtimebuddy bigtimebuddy merged commit 356abaa into dev May 24, 2023
3 checks passed
@bigtimebuddy bigtimebuddy deleted the fix/extractThrowsIfFrameIsEmpty branch May 24, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants