Skip to content

Conversation

@hannahbergam
Copy link
Contributor

@hannahbergam hannahbergam commented Jun 9, 2021

Because paint can "glom" into squares it isn't actually "painted on", paint may clash with assets. As previously written, the paint would sit just on top of those assets, leaving a corner of the bench, car, etc. covered with color. This PR moves the paint just below the assets. It also fixes the formatting of the existing drawer to match "prettier" formatting.

There were a few possible approaches for this, and I went with number 3:

  1. Mathematically/logically calculate where the assets were in relation to the paint, and only create the "paint" svg if there is not an asset currently in that tile. This would have been complicated to read and write, and generally confusing because the svg grid has an offset, so the tile id's don't exactly align with actual grid id's (and not to mention, the rotation values break this alignment as well).
  2. Render the assets after the paint, because the order of rendering determines the layering. This isn't ideal because the assets are currently rendered during drawMapTiles at the very beginning, before any paint has been added. So it would involve creating a duplicate set of assets to go over the paint after its application. Creating duplicate assets seems like a quick way to crash a browser in the case of infinite loops.
  3. When rendering paint, grab the existing assets and move them to the end of the document (after the paint) so they appear on top.

I achieved this by keeping track of an assetList, which is comprised of the entire list of asset id's as they are rendered. This way, each time we paint, we can quickly iterate back through that list, finding all the assets by id in the document and moving them just before the pegman (at the end) so we can assure they will sit "on top of" the paint.

Jira task: https://codedotorg.atlassian.net/browse/CSA-430

After:
image

Before:
image

let node = document.getElementById(asset);
let pegmanElement =
this.svg_.getElementsByClassName("pegman-location")[0];
this.svg_.insertBefore(node, pegmanElement);
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you able to verify that this moves rather than duplicates the node? (Sorry for asking - my knowledge of DOM manipulation isn't very strong - just double checking)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I went back to check and it was just moving all the assets to the end:)

Copy link
Contributor

@jmkulwik jmkulwik left a comment

Choose a reason for hiding this comment

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

Not needed for this change, but in the future merge lint-only changes separate from functional changes. It makes reviewing much faster and easier.

@hannahbergam
Copy link
Contributor Author

Not needed for this change, but in the future merge lint-only changes separate from functional changes. It makes reviewing much faster and easier.

🙌 Will do. Thanks!

@hannahbergam hannahbergam merged commit 517878c into main Jun 9, 2021
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.

2 participants