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

Random Terrain Tile #283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marinho
Copy link

@marinho marinho commented May 24, 2021

I have made this new tile system, which is a copy of Terrain Tile, but with random behaviour for all given levels. It also includes a schema that facilitates how to draw a layout and position tiles in the expected rotation.

This one is based on version 1.5.0-preview, as it's the one that worked for my version of Unity.

I'd like you to appreciate this pull request. If I get positive feedback, I can update it to the latest version on master branch, and add more documentation and tests, if necessary/possible.

@unity-cla-assistant
Copy link

unity-cla-assistant commented May 24, 2021

CLA assistant check
All committers have signed the CLA.

}

private Sprite[][] GetSpritesAsOne() {
Sprite[][] m_Sprites = new Sprite[15][];
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this could be a switch statement returning the correct Sprite Array when the index is passed it. This would avoid creating a new array of arrays each time.

return (tile != null && tile == this);
}

private int GetIndex(byte mask)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and GetTransform below is used by the original Terrain Tile itself. Perhaps we could consider making RandomTerrainTile inherit from TerrainTile and try to share as much common functions as possible?

@@ -0,0 +1,33 @@
# Random Terrain Tile

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for writing the documentation for this!

A small addition would be adding a contribution note with your username too, for example in https://github.com/Unity-Technologies/2d-extras/blob/master/Documentation~/LineBrush.md . It would be good acknowledgement of your work here!

// }


// public override void OnInspectorGUI()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having a nice inspector would be great if possible!

Copy link
Collaborator

@ChuanXin-Unity ChuanXin-Unity left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and good work! I have added a few comments which I think could help improve this a little more. Do let us know what you think about it!

@marinho
Copy link
Author

marinho commented May 30, 2021

hi @ChuanXin-Unity , thank you for the feedback! I'm going to do those corrections soon. Have a nice week!

@marinho
Copy link
Author

marinho commented May 31, 2021

hi @ChuanXin-Unity , I have a question, though: as this repository is no longer maintained, as it's mentioned in README.md, what happens to these PRs?

I mean, my plan is to update this branch to latest version of master and improve it following your review, but is it going to be merged anyhow?

thank you in advance

@ChuanXin-Unity
Copy link
Collaborator

The notice in the README.md is based of the latest directive from our superiors, and they are still currently in discussions on what to do with this on the whole. Unfortunately, there is nothing concrete yet, other than it is possible that this repository could be made internal.

For now, you can continue working by updating the branch to the latest version of master and updating the PR as well, we will still continue to take and look at the PR and accept it to the repository.

Thanks!

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.

3 participants