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

Javalab: add serialized_maze property #40603

Merged
merged 4 commits into from
May 17, 2021
Merged

Conversation

molly-moen
Copy link
Contributor

For neighborhood levels in Javalab we need to specify a serialized_maze property, which defines the map for a neighborhood level. This property will be required for any Javalab level with csa_view_mode='neighborhood'. The expected format is a 2d array of objects, where each object must have a tileType (number Maze recongizes as wall, obstacle, open, etc.) and may optionally have an assetId and/or value.

Testing story

Tested locally using in-progress work on the Maze repo that renders neighborhood levels.

Follow-up work

We will need to add a new route on levles so that Javabuilder can access the maze as well, as it needs to know the grid setup in order to build the environment for neighborhood on the backend.

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@molly-moen molly-moen requested a review from a team May 14, 2021 19:21
Copy link
Contributor

@hannahbergam hannahbergam left a comment

Choose a reason for hiding this comment

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

Looks good to me! I've been wanting to ask more about testing- I know for levelbuilder changes we don't have a good UI test setup, but should we be focusing on making a more thorough unit test collection?

end
return if serialized_maze.nil?
# convert maze into json object and validate each cell has a tileType
maze_json = serialized_maze.is_a?(Array) ? serialized_maze.to_json : serialized_maze
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason (or could there be in the future) the serialized_maze wouldn't be of type Array or already in JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at how it's defined for other levels, it's generally a string which we need to call JSON.parse on. I copied this behavior from how maze parses serialized_maze. I think the only way it would ever be defined as an array is if an engineeer was editing a level manually. Both are parseable (array or escaped string), so seems reasonable to allow both unless we have strong opinions otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha- thanks for the context. That makes more sense. I was wondering if there would be any other formats to worry about, but it sounds like it's a pattern that's been working for maze already!

@molly-moen
Copy link
Contributor Author

@hannahbergam good call out on testing. I will add a javalab_test file--previously the logic was basic enough to be covered by our generic level tests, but it would be good to test this new functionality explicitly.

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.

LGTM! Thanks for picking this up!

@molly-moen molly-moen merged commit 4635607 into staging May 17, 2021
@molly-moen molly-moen deleted the javalab-add-serialized-maze branch May 17, 2021 20:50
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

3 participants