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

Banner Patterns #7216

Open
wants to merge 3 commits into
base: dev/feature
Choose a base branch
from

Conversation

TheAbsolutionism
Copy link

Description

This PR aims to allow users to manipulate banner patterns on a banner

Note: Yes I do know the banner pattern exists within skript-aliases. However, it does not allow setting the color of the pattern.


Target Minecraft Versions: any
Requirements: none
Related Issues: none

Comment on lines +71 to +73
patterns.add("[a] [new] "
+ getKey(type)
+ " [banner] pattern colo[u]red %*color%"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
patterns.add("[a] [new] "
+ getKey(type)
+ " [banner] pattern colo[u]red %*color%"
patterns.add("[a] %*color%"
+ getKey(type)
+ " [banner] pattern"

can use the old pattern for non-literals

Object type = patternTypes[i];
try {
patterns.add("[a] [new] "
+ getKey(type)
Copy link
Member

Choose a reason for hiding this comment

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

why not just register a new type for the patterns?

Copy link
Author

Choose a reason for hiding this comment

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

Do you think this would be better? I assumed having all this mess with reflection and stuff would be better to have in this class rather than in the BukkitClasses

Copy link
Member

Choose a reason for hiding this comment

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

well i would make a banner module honestly
but yes it would be way better

Copy link
Author

Choose a reason for hiding this comment

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

The only problem I see by adding the type, is that it may conflict with the banner patterns within skript-aliases. If the patterns I set within the lang consist of the words banner pattern such as creeper: creeper banner pattern

Copy link
Member

Choose a reason for hiding this comment

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

try it and see

Copy link
Author

Choose a reason for hiding this comment

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

That is true, so how should we or I approach this?

Copy link
Member

Choose a reason for hiding this comment

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

well there's precedent here with stuff like chicken and a chicken
since you can do 1 of creeper banner pattern to avoid the ambiguity i'm willing to accept the breaking change
idk about the others' opinions.

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, we could get wacky and use unique names, like stamp instead of pattern, or something similarly different.

Copy link
Author

Choose a reason for hiding this comment

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

Im down with whatever, what about creeper pattern type?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of just appending type to things, as it doesn't really mean much in general and is already confusing enough with enchantment vs enchantment type and same with potion effects. I'm in favor of as-is, with the breaking change, tbh.

@sovdeeth sovdeeth added the feature Pull request adding a new feature. label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants