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

Mod Material support, for Hybrid Server #7126

Open
wants to merge 13 commits into
base: dev/patch
Choose a base branch
from

Conversation

0XPYEX0
Copy link
Contributor

@0XPYEX0 0XPYEX0 commented Oct 2, 2024

Description

As title, support for items which added by mods
For PR: #7121

on enchant:
    if event-item is (cataclysm's the incinerator): # The item ID
        send "success" to player

b258fdd942e5bf2210428d7b21cdb3d9

on right click:
    send "%type of tool of player%" to player

63134e0a5c43b268706222ad750c48d0
It's an old screenshot. Now it's quack's abacus

The pattern just like: mod:an_item_name_with_line (parsed to alias)→ mod's an item name with line
And vanilla aliases are not changed. minecraft:dirtdirt


Target Minecraft Versions: any
Requirements: none
Related Issues: #4051 #4678 #4778 #6503

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Oct 2, 2024
@BredyAK
Copy link

BredyAK commented Oct 2, 2024

I strongly recommend to approve this PR because:

  1. in future minecraft versions, maybe there will be a way to directly add blocks/items to minecraft by datapack. then the namespace may not be minecraft. but currently Skript doesn't support to create aliases for items which do not use minecraft as their namespace.
  2. as the 1st opinion said, if this PR is approved, then Hybird servers will also benefit from this, which could let modded items being added to Skript aliases.
  3. this PR doesn't break any Skript's current core things, but increased scalability.
  4. there are a lot of people want this too, such as https://forums.skunity.com/threads/14952/, https://forums.skunity.com/threads/9848/, https://forums.skunity.com/threads/14457/, etc. if you google "Skript Mod Item" or something similiar, you could find a lot more.

i know that Skript doesn't support Hybird servers, but this PR is intended to improve the scalability of Skript, not just for Hybird servers.

(ref: #7121 (comment))

If we check it, Skript cannot parse aliases of mod items (In Hybrid Server

(ref: #7121)

good job! your unrealized plan in the previous PR was finally realized, while ensuring the core functions of Skript.

Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

src/main/java/ch/njol/skript/bukkitutil/BukkitUnsafe.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/bukkitutil/BukkitUnsafe.java Outdated Show resolved Hide resolved
@BredyAK
Copy link

BredyAK commented Oct 2, 2024

i just did some tests, no worry, all of them are runned as expected:

  1. no matter what the option load default aliases in Skript's config file is set (true or false), it doesn't affect vanilla or mod's aliases' generation.
  2. when i execute !give cataclysm's the incinerator to player, i successfully get the expected item, and its id is cataclysm:the_incinerator.
  3. when i execute !give diamond chestplate to player, i successfully get the expected item, and its id is minecraft:diamond_chestplate.

this is an epic PR for me, thanks! this is why i love Skript's community.

@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Oct 2, 2024

Hey guys, maybe we could do like this?
image
I add a of expression, and make it looks like quack mod's abacus or abacus of mod quack
Should it be done or not?

@sovdeeth @Efnilite

@sovdeeth
Copy link
Member

sovdeeth commented Oct 2, 2024

Hey guys, maybe we could do like this?

I'm not a fan of the |s stuff, but i suppose mod's x and x from mod is ok
actually ignore the first part

@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Oct 2, 2024

Hey guys, maybe we could do like this?

I'm not a fan of the |s stuff, but i suppose mod's x and x from mod is ok

Just like abacus from mod quack, right? Not of ?

@sovdeeth
Copy link
Member

sovdeeth commented Oct 2, 2024

Hey guys, maybe we could do like this?

I'm not a fan of the |s stuff, but i suppose mod's x and x from mod is ok

Just like abacus from mod quack, right? Not of ?

I'd like <item> from <mod>, so abacus from quark

@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Oct 2, 2024

Hey guys, maybe we could do like this?

I'm not a fan of the |s stuff

Actually idk what is it, but there was a ¦s before, so I kept it

@sovdeeth
Copy link
Member

sovdeeth commented Oct 2, 2024

Hey guys, maybe we could do like this?

I'm not a fan of the |s stuff

Actually idk what is it, but there was a ¦s before, so I kept it

ignore that message, it's fine to have

@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Oct 2, 2024

c1644026a8294ce6503dc6fcdfb2bedc
Successfully to get cataclysm:the_incinerator

@Fusezion
Copy link
Contributor

Fusezion commented Oct 2, 2024

I'm a bit concerned on how many aliases this will add if it's 3 per modded item, skript already suffers enough from how many they just add.

@BredyAK
Copy link

BredyAK commented Oct 2, 2024

I'm a bit concerned on how many aliases this will add if it's 3 per modded item, skript already suffers enough from how many they just add.

one point: they are not generated for everyone. with this PR, it will just create aliases for things that ALREADY in your server. if you don't have mod items (like vanilla server), it won't affect you at all.

@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Oct 2, 2024

I'm a bit concerned on how many aliases this will add if it's 3 per modded item, skript already suffers enough from how many they just add.

Not 3 but 2. And this is not the reason to make a language not support mod items. That's not fair😰
If you are running a vanilla server, this will not affect you anymore
If you are running a Hybrid server, this should be your good news 😊

@sovdeeth
Copy link
Member

sovdeeth commented Oct 2, 2024

I'm a bit concerned on how many aliases this will add if it's 3 per modded item, skript already suffers enough from how many they just add.

well it's 4 per item, vs 2 per item for the vanilla items
even with 10,000 materials, that's only 40,000 aliases, which is pretty cheap
The thing that makes aliases take so long is having hundreds or thousands of aliases for a single item, like stairs.

@BredyAK
Copy link

BredyAK commented Oct 2, 2024

I'm a bit concerned on how many aliases this will add if it's 3 per modded item, skript already suffers enough from how many they just add.

here is what this PR do:

  1. if you are a vanilla server, then just ignore this PR because it take no effect of your server.
  2. if you are a hybird server, then this PR will make Skript intelligently generate the aliases it needed. not only all the vanilla aliases will be kept, but also help you generate the extra aliases such as created by your mods, etc.

in future, maybe Mojang will provide a way to let creators add blocks/items/etc. to Minecraft by simply using datapakcs (like Bedrock Edition's behavior pack + resource pack), then creators could use their own nameplaces (not minceraft:), Skript will become more scalable then.

@Efnilite
Copy link
Member

Efnilite commented Oct 2, 2024

I'm a bit concerned on how many aliases this will add if it's 3 per modded item, skript already suffers enough from how many they just add.

And this will be made less bad by #7084 :)

@Fusezion
Copy link
Contributor

Fusezion commented Oct 2, 2024

one point: they are not generated for everyone. with this PR, it will just create aliases for things that ALREADY in your server. if you don't have mod items (like vanilla server), it won't affect you at all.

Thank but I'm aware I know what this PR does and I'm aware what I said

Not 3 but 2. And this is not the reason to make a language not support mod items. That's not fair
If you are running a vanilla server, this will not affect you anymore
If you are running a Hybrid server, this is your good news

Must of missed something, thought it was 3. I never said we should scrap this whole thing just because of that.
It's just a fact skript still by default registers 233k in their own in addition to this

well it's 4 per item, vs 2 per item for the vanilla items
even with 10,000 materials, that's only 40,000 aliases, which is pretty cheap
The thing that makes aliases take so long is having hundreds or thousands of aliases for a single item, like stairs.

Wait wait it's 4 now? And no it really isn't cheap for 40k that's still a major concern granted it should be rare for anyone to reach 10k items, so I'm fine with stepping down on this but none the less it's concerning.

If anything a config option should still be added similar to vanilla minecraft items but only accept namespace key path and remove x's item and item from x

@sovdeeth
Copy link
Member

sovdeeth commented Oct 2, 2024

well it's 4 per item, vs 2 per item for the vanilla items
even with 10,000 materials, that's only 40,000 aliases, which is pretty cheap
The thing that makes aliases take so long is having hundreds or thousands of aliases for a single item, like stairs.

Wait wait it's 4 now? And no it really isn't cheap for 40k that's still a major concern granted it should be rare for anyone to reach 10k items, so I'm fine with stepping down on this but none the less it's concerning.

40k is very cheap for 10k items, honestly. The benefit from having basic grammatical aliases far outweighs the slight reduction in alias count, imo.
and it's 4 because of plurality, the |s adds a second plural alias, so vanilla is 2 and modded is 4

If anything a config option should still be added similar to vanilla minecraft items but only accept namespace key path and remove x's item and item from x

There is no such config option for vanilla items that does that, though.

and RE: the 233k current ones, those will not be included by default in 2.10, only the ~4000 auto generated ones for the ~2000 items minecraft has

@BredyAK
Copy link

BredyAK commented Oct 2, 2024

idk if there is a need to add a config option as @Fusezion said, but i could post my server log for reference:

[02:55:01 INFO]: [Skript] You're currently running a custom Skript version. No updates will be automatically installed.
[02:55:10 INFO]: [Skript] Loaded 260276 aliases in 8747ms

my server has 260000+ aliases and it loaded for 8747ms.

@Pikachu920
Copy link
Member

I don't see a need for a config option, but I do want to ask whether we should add a warning when running on a modded server? We don't support modded platforms, so it is probably not a good idea for people to be using skript to build their modded servers. I know people will do it anyway, but I'd love to at least make it clear they are on their own.

@BredyAK
Copy link

BredyAK commented Oct 2, 2024

I don't see a need for a config option, but I do want to ask whether we should add a warning when running on a modded server? We don't support modded platforms, so it is probably not a good idea for people to be using skript to build their modded servers. I know people will do it anyway, but I'd love to at least make it clear they are on their own.

i don't think there is a need to add warnings... because it doesn't affect vanilla server at all.
and you are using hybird server, how couldn't you know the warning things?

btw,

We don't support modded platforms

this is not a thing only about modded platforms. i could use my reply above:

I'm a bit concerned on how many aliases this will add if it's 3 per modded item, skript already suffers enough from how many they just add.

here is what this PR do:

  1. if you are a vanilla server, then just ignore this PR because it take no effect of your server.
  2. if you are a hybird server, then this PR will make Skript intelligently generate the aliases it needed. not only all the vanilla aliases will be kept, but also help you generate the extra aliases such as created by your mods, etc.

in future, maybe Mojang will provide a way to let creators add blocks/items/etc. to Minecraft by simply using datapakcs (like Bedrock Edition's behavior pack + resource pack), then creators could use their own nameplaces (not minceraft:), Skript will become more scalable then.

@EquipableMC
Copy link
Contributor

I don't see a need for a config option, but I do want to ask whether we should add a warning when running on a modded server? We don't support modded platforms, so it is probably not a good idea for people to be using skript to build their modded servers. I know people will do it anyway, but I'd love to at least make it clear they are on their own.

I feel like we should, because some modded platforms mess with some internal code so yeah, and plus if people go to open an issue about it, they may see that yellow warning message and not do it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants