Skip to content

Only check position against effect transform if it falls within the Drawable's space #424

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 30, 2019
Merged

Only check position against effect transform if it falls within the Drawable's space #424

merged 1 commit into from
Apr 30, 2019

Conversation

ktbee
Copy link
Contributor

@ktbee ktbee commented Mar 18, 2019

Resolves

#366

Proposed Changes

Check if the local position is within the drawable's space (ie the x and y values are equal to zero or less than one) before transforming that point using EffectTransform.

Reason for Changes

If we use the effect transform on points outside the drawable, it can falsely give the impression that a point is within a drawable when you transform it. Without this if-statement, blocks like "touching mouse-pointer" will think a transformed point is part of the drawable if the mosaic effect returns a positive modded value here

Here is a project that demos the fix: https://scratch.mit.edu/projects/293415549/editor

After clicking the green flag, click the block to add the mosaic effect. The sprite will change colors to the bottom right of it in production, even if the mouse isn't touching the sprite. With these fixes, it will only change color when it's actually being touched.

@ktbee ktbee requested review from kchadha and cwillisf March 18, 2019 19:42
@thisandagain thisandagain added this to the March 2019 milestone Mar 18, 2019
@thisandagain
Copy link
Contributor

/cc @kchadha

@ktbee ktbee changed the title Only check local position against mosaic effect transform if it falls within the Drawable's space Only check local position against effect transform if it falls within the Drawable's space Mar 19, 2019
@ktbee ktbee changed the title Only check local position against effect transform if it falls within the Drawable's space Only check position against effect transform if it falls within the Drawable's space Mar 19, 2019
cwillisf
cwillisf approved these changes Apr 30, 2019
View reviewed changes
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

👍

@cwillisf cwillisf modified the milestones: March 2019, April 2019 Apr 30, 2019
@cwillisf cwillisf merged commit 4a55d63 into scratchfoundation:develop Apr 30, 2019
@ktbee ktbee deleted the limit-mosaic-effect branch May 13, 2019 16:00
@ktbee ktbee mentioned this pull request May 13, 2019
Closed
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Mar 15, 2021
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
8f0b549 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Mar 15, 2021
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
fe2dce5 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Mar 15, 2021
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
c823982 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Mar 15, 2021
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
c158b49 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Mar 15, 2021
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
84c5d75 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Mar 15, 2021
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
0368955 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Mar 15, 2021
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
8aaf96c 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Jun 30, 2021
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
428001d 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Feb 21, 2024
@adroitwhiz
Move effect transform + bounds check out of getLocalPosition
8957537 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Feb 21, 2024
@adroitwhiz
refactor: move effect transform + bounds check out of getLocalPosition
d254525 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
adroitwhiz added a commit to adroitwhiz/scratch-render that referenced this pull request Feb 21, 2024
@adroitwhiz
refactor: move effect transform + bounds check out of getLocalPosition
9a9ebb2 
This results in a tad bit of duplicated code, but considering that we
have 4 different code paths (colorAtNearest, colorAtLinear,
isTouchingNearest, and isTouchingLinear) for sampling a silhouette,
that's to be expected.

This more closely matches the GPU pipeline, in which color and position
calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424
with a solution that matches the GPU: instead of not transforming points
outside the skin bounds, just return transparency/false early.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwillisf cwillisf cwillisf approved these changes

@kchadha kchadha Awaiting requested review from kchadha

Assignees

@cwillisf cwillisf

Labels
compatibility pr - good to merge
Projects
None yet
Milestone
April 2019
Development

Successfully merging this pull request may close these issues.
3 participants
@ktbee @thisandagain @cwillisf