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

Artist draw pattern backward #20989

Merged
merged 9 commits into from Mar 5, 2018
Merged

Artist draw pattern backward #20989

merged 9 commits into from Mar 5, 2018

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented Mar 2, 2018

Bug: Artist does not draw any pattern backwards.
pre-fix-pattern

Solution: The following changes were implemented:

  • Added two variables for x and y coordinates for clipping the image to allow for future updates.

  • Changed the x coordinate for clipping the image from 0 to image's width/2. Prior to fix, x and y coordinates were (0, 0). Therefore, if a distance of -50 px is selected, a blank space is clipped resulting in no image being rendered. To resolve this, the start point for clipping the image is now set at the mid-point (image's width/2), which ensures the selection of backward or forward movement starts at the same point.

post-fix-pattern

@caleybrock
Copy link
Contributor

Question for CSF - does validation rely on the exact output of the simulation? My understanding is that this might change the starting point of the pattern when moving forward?

@Hamms
Copy link
Contributor

Hamms commented Mar 2, 2018

Validation does indeed depend on the exact output of the simulation (we actually do pixel-by-pixel matching), but we specifically compare this output to the result of running the solution code. So as long as the starting point of the pattern changes consistently for both user code and solution code, this should be fine.

@Hamms
Copy link
Contributor

Hamms commented Mar 2, 2018

Test failure looks like a side effect of this branch being created from staging a while ago. If you update staging locally, then git rebase staging this branch it should be fine

@balderdash
Copy link
Contributor

The rainbow pattern asset is 800px long, so I don't think this will work if you draw a diagonal across the whole canvas. I.e. if you jump to (0, 0), turn 45 degrees, and move forward 500 pixels, you'll run out of space in the pattern asset.

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

This looks great!

// clip region size
distance + img.height / 2, img.height,
distance, img.height,
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR is great! I wonder if we should keep the nonsensical + img.height / 2 as-is for this PR, and open a follow up PR with that change?

Copy link
Contributor

@Erin007 Erin007 left a comment

Choose a reason for hiding this comment

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

I don't know much about Artist code, but you've pulled in the right experts to review. The PR description is thorough - nice screenshots! Your change is well scoped in a digestible chunk, and the test coverage is great. ✅

@nkiruka
Copy link
Contributor Author

nkiruka commented Mar 5, 2018

Updated PR based on suggestions. Tests also updated. Please review.

Copy link
Contributor

@balderdash balderdash left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple small things to clean up.

@@ -228,6 +228,10 @@ class Visualization {
this.turtleFrame_ = 0;
}

clipImage() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you didn't end up using this function, so you can remove it.

@@ -1510,7 +1524,7 @@ Artist.prototype.step = function (command, values, options) {
result = this.calculateSmoothAnimate(options, distance);
tupleDone = result.tupleDone;
this.visualization.setHeading(heading);
this.visualization.jumpForward(result.distance);
this.jumpForward(result.distance);
Copy link
Contributor

Choose a reason for hiding this comment

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

This changed with Josh's recent refactoring, it should be this.visualization.jumpForward(...

@nkiruka nkiruka merged commit 8246e8a into staging Mar 5, 2018
@nkiruka
Copy link
Contributor Author

nkiruka commented Mar 5, 2018

Oops, I didn't mean to merge PR. I will revert back to previous PR.

@nkiruka nkiruka deleted the artist-draw-pattern-backward branch September 17, 2018 18:21
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

6 participants