-
Notifications
You must be signed in to change notification settings - Fork 15
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
Improved Sign.json #42
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review individual comments and consider making changes accordingly.
@@ -241,11 +242,10 @@ | |||
"variant": "39" | |||
}, | |||
{ | |||
"name": "Toll Ahead", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with making this change. If a player has a map where they used a ton of 40s instead of 37s, then it's going to require them to go through and update all of those signs for no real good reason. The problem is Minecraft doesn't have a good "migration" system where we can just change these for the player so until we can figure that out, I don't think we can do this.
@@ -207,6 +207,7 @@ | |||
"name": "Oncoming Traffic Has Priority Over You", | |||
"type": "circle", | |||
"variant": "33" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma
"type": "circle", | ||
"variant": "40", | ||
"tooltip": "To be removed", | ||
"note": "This is a duplicate of Circular Sign 37 and will be removed." | ||
"tooltip": "This sign was a duplicate of Circle 37. Please use that.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comma
@@ -1643,28 +1643,24 @@ | |||
"type": "misc", | |||
"variant": "33", | |||
"tooltip": "Use to show the general direction of the route.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comma
@@ -1643,28 +1643,24 @@ | |||
"type": "misc", | |||
"variant": "33", | |||
"tooltip": "Use to show the general direction of the route.", | |||
"note": "Should be used with route markers to show cardinal direction. The exact direction of the road needs not be north, but the route should go in that general direction." | |||
}, | |||
{ | |||
"name": "'SOUTH' Plaque", | |||
"type": "misc", | |||
"variant": "34", | |||
"tooltip": "Use to show the general direction of the route.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comma
}, | ||
{ | ||
"name": "'SOUTH' Plaque", | ||
"type": "misc", | ||
"variant": "34", | ||
"tooltip": "Use to show the general direction of the route.", | ||
"note": "Should be used with route markers to show cardinal direction. The exact direction of the road needs not be south, but the route should go in that general direction." | ||
}, | ||
{ | ||
"name": "'EAST' Plaque", | ||
"type": "misc", | ||
"variant": "35", | ||
"tooltip": "Use to show the general direction of the route.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comma
}, | ||
{ | ||
"name": "'EAST' Plaque", | ||
"type": "misc", | ||
"variant": "35", | ||
"tooltip": "Use to show the general direction of the route.", | ||
"note": "Should be used with route markers to show cardinal direction. The exact direction of the road needs not be east, but the route should go in that general direction." | ||
}, | ||
{ | ||
"name": "'WEST' Plaque", | ||
"type": "misc", | ||
"variant": "36", | ||
"tooltip": "Use to show the general direction of the route.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comma
"name": "SPAWN AREA, No PVP", | ||
"type": "square", | ||
"variant": "172" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma
@@ -3936,6 +3932,452 @@ | |||
"type": "triangle", | |||
"variant": "95" | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole inserted area overlap with already existing variant numbers and therefore isn't doing anything. I'm not 100% sure what you're trying to do here, either. It seems like this is a mix of signs that have already been added but have a different variant number or you're trying to add new signs, which 1) are missing textures in this PR, and 2) should be done through other avenues rather than done through a PR, though I appreciate you trying to help put them in.
A fork or branch for a past update had an improved sign.json file with shorter, more concise notes. I modified some of those notes. However, now that there have been multiple updates, there are now duplicate entries in the sign.json file that need to be corrected.