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

Blossoming Pots #1070

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

runcows
Copy link

@runcows runcows commented Nov 24, 2024

Blossoming Pots initial commit.
Ready for code review and further discussion, I think...

@misode misode added submission Brand new community submitted module needs-testing Requires in-game testing labels Nov 24, 2024
Copy link
Member

@misode misode left a comment

Choose a reason for hiding this comment

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

I would probably want to limit the number of display entities per pot to something like 5. Anything above that I feel goes beyond of what "feels vanilla". Especially the coral decorations, where it's using 41 display entities for a single pot. That's not only a performance issue, but I also don't think it looks good visually, since there are too many small details and you just don't see that with vanilla. This already has the coral fans, I think those are a better fit.

I was a bit unsure on how this module worked tech wise, with the storage. From my point of view coming from beet, I would probably generate a mcfunction for each supported item, which just had the item display summon commands, that would probably allow you more flexibility without confirming to a strict storage schema.
I think it is probably fine to keep using the storage, unless you come across a major hurdle.

I didn't do a full technical review yet, but it seems fairly well put together after a first look.

@runcows
Copy link
Author

runcows commented Nov 27, 2024

I would probably want to limit the number of display entities per pot to something like 5. Anything above that I feel goes beyond of what "feels vanilla". Especially the coral decorations, where it's using 41 display entities for a single pot. That's not only a performance issue, but I also don't think it looks good visually, since there are too many small details and you just don't see that with vanilla. This already has the coral fans, I think those are a better fit.

Totally get it. I feel a little bad removing the coral blocks because I worked hard on them when I made them, but I totally get it and am down to axe them. I'm curious on your thoughts for a limit of display entities. There's only 5 other things that have more than 5 displays. The next highest count after Coral Blocks is the 3rd stage of Chorus Fruit with 18, which im much more hesitant to remove. After that is Kelp and Twisting Vines which go up to 16 and I'm willing to chop those down a good bit. Bamboo and Glow Berries have 8 and Weeping Vines has 9, again willing to chop those down a little bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-testing Requires in-game testing submission Brand new community submitted module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants