mc-datapacks / review-tracker

Datapack Review tracker
6 stars 1 forks source link

S.E.A.T Pack - Sit anywhere in Minecraft #50

Closed x-2mas closed 3 years ago

x-2mas commented 3 years ago

Project Page: Planet Minecraft

Notes:

When it comes to namespaces, I had already implemented a 'x_' namespace for all pack features before learning about Datapack Conventions. That namespace feels lazy but I chose that in order to fit the scoreboard 16-char limit. I wish I had used a more verbose namespaces like 'xmas' or 'xms' but I feel its too late to change it now. The C.O.R.E features a backward-compatibility versioning system that might break if I did that.

If you find the code too complex, have a look in the 'scripts' folder where I've commented pretty much every line of code there is. The scripts follow the mcscript syntax found here. I used mcscript for code organization reasons but it all compiles to .mcfunctions in the end.

If you need anymore help, please reach out to me! Thank You! πŸ’›

x-2mas commented 3 years ago

So sorry to do this, but I just realized there was some C.O.R.E code that didn't conform to standards. I missed it because I wasn't actively using it in my S.E.A.T pack.

Should be all updated now! Download

Changes:

x-2mas commented 3 years ago

Alright, the last update I made, which added global.ignore tags, ended up breaking mob-riding features (I made my code ignore its own entities). The latest update fixes that.

Changed Files:

PS: I have a feeling this is going to keep happening. Sorry about that. I'll mention any updates/changes here.

EstEarth202 commented 3 years ago

Certified Compatibility ❓

Stamp of Quality ❓

πŸ’•β”β”β”β”β”γƒ»Stampγƒ»β”β”β”β”β”πŸ’•

Datapack Advancement βœ”οΈ

I think you put it alternately.

"title": "Xmas Labs - S.E.A.T",
            "item": "minecraft:nether_star"
"title": "Xmas Labs - C.O.R.E",
            "item": "minecraft:saddle"

Datapack Uninstallation βœ”οΈ

Reloading Message ❓ (Recheck after Tecnical advice fix)

πŸ”†β”β”β”β”β”γƒ»Not Forceγƒ»β”β”β”β”β”πŸ”†

Common Trait Convention β˜‘οΈ

Global Durability β˜‘οΈ

βš β”β”β”β”β”γƒ»Important γƒ»β”β”β”β”β”βš 

Custom Model ID β˜‘οΈ

Global ignoring tag ❓(Recheck after Tecnical advice fix)

Namespace Convention ❌

❔ Which you can choose not to do if you want, you can tell me if you want. Because this is not force and does not affect reviews. But the impact on the review will be in an important section

Since you use mcscript to write this pack, you can use it fully. But that's not a problem But the real problem is with the compile output. Which is technically very detrimental to performance. And as a result, reviewing some points is difficult So I want you to fix this before I add another review.

Your datapack runs multiple lines of commands that have the same conditions. Which I will give a rough example as follows:

What effect does this have? It results in When it's done running the first command, it needs to go unnecessarily return to as @selector with the same condition again. Code_-_Insiders_GSVIFdrB7E Instead of Code_-_Insiders_LxZdpthKA7 ↳ Code_-_Insiders_DgGe6xp7BK

There are still a lot of mcfunction files like this. That should be fixed.

If you are looking for a sample data pack in terms of performance and organization of files, folders and commands. I suggest you to watch the work of Boomber360

πŸ“β”β”β”β”β”γƒ»Noteγƒ»β”β”β”β”β”πŸ“

βœ”οΈ = Followed πŸ”† = Advised to use ❌ = Doesn't follow ❓ = Doubt / Something missing β˜‘οΈ = Not required/Skip

x-2mas commented 3 years ago

Thank you for your review!! I've corrected the namespace issue as well as the advancement item mix-up (lol). Updated here.

As for the last point about optimizing commands, you are absolutely right! Using the same condition prefixes over and over will significantly affect performance. What's funny is that this is the only feature of mcscript that I use! It's called command-grouping and it converts this:

  as('@e[tag=demo]'), if('score @s DEMO matches 1..'){
    /say Demo!
    /say Another Demo!
    /say Another Another Demo!
  }

to this:

  /execute as @e[tag=demo] if score @s DEMO matches 1.. run say Demo!
  /execute as @e[tag=demo] if score @s DEMO matches 1.. run say Another Demo!
  /execute as @e[tag=demo] if score @s DEMO matches 1.. run say Another Another Demo!

As you pointed out, this is indeed very inefficient. As a result, I plan to completely remove mcscript and rewrite both datapacks using only Minecraft functions. I have to do this especially since mcscript is no longer being developed and a lot of its features are broken in current versions of Minecraft.

Unfortunately, I'll only get time to do this when I work on my next datapack (which won't be anytime soon) and since this isn't a requirement for certification, I'm going to leave it like this for now. I hope that's okay? πŸ˜…

Thanks again for taking the time to review this! I know this must be a challenge to review so feel free to keep this your lowest priority. :)

EstEarth202 commented 3 years ago

No problem, you can update your performance whenever you want. After I finished the review When you correct those points, you can checkbox to inform other users who have to see this detail.

EstEarth202 commented 3 years ago

Certified Compatibility ❌

Stamp of Quality ❌

πŸ’•β”β”β”β”β”γƒ»Stampγƒ»β”β”β”β”β”πŸ’•

Datapack Advancement βœ”οΈ

Datapack Uninstallation βœ”οΈ

Reloading Message ❌

❔ Which you can choose not to do if you want, you can tell me if you want. Because this is not force, but will affect the Stamp of Quality.

I can see that this pack has a tellraw message at installation, which shouldn't be there for this. You should be able to use other methods instead.

πŸ”†β”β”β”β”β”γƒ»Not Forceγƒ»β”β”β”β”β”πŸ”†

Common Trait Convention β˜‘οΈ

Global Durability β˜‘οΈ

βš β”β”β”β”β”γƒ»Important γƒ»β”β”β”β”β”βš 

Custom Model ID β˜‘οΈ

Global ignoring tag ❌

You don't have a global.ignore at @e using score condition, because score is less clear than tag and nbt. Therefore cannot be excluded to not have

data\x_core\functions\ticker.mcfunction
data\x_core\functions\scrollbar.mcfunction

data\x_core\functions\rotation_tracking.mcfunction -> Other: `data get entity @s Rotation[X] 1` I see that you put X

data\x_core\functions\manage_eids.mcfunction
data\x_core\functions\manage_attachments.mcfunction
data\x_core\functions\collision_blocking.mcfunction

Namespace Convention βœ”οΈ

Shulker Box Loot Table β˜‘οΈ

πŸŽ―β”β”β”β”β”γƒ»Other γƒ»β”β”β”β”β”πŸŽ―

Something missing ❓

Bug report 🐞

data\x_seat\functions\seat_surface.mcfunction

@a[tag=x_SEAT_SURFACE_PLAYER,nbt={SelectedItem:{id:"minecraft:hay_bale"}}] ->Here, the extension warns that this is wrong.

data\x_seat\functions\seat.mcfunction -> unless entity @s[nbt={SelectedItem:{id:"minecraft:hay_bale"}}]

Which I think it should be minecraft: hay_block

In the datapack enable x_seat after "file / x_core" command there will be a problem. The command will not work if the names do not match. Both in the case of a folder and a .zip

πŸ“β”β”β”β”β”γƒ»Noteγƒ»β”β”β”β”β”πŸ“

βœ”οΈ = Followed πŸ”† = Advised to use ❌ = Doesn't follow ❓ = Doubt / Something missing β˜‘οΈ = Not required/Skip

x-2mas commented 3 years ago

Thank you for the review. Files updated here.

Changes:

Now about the other changes, can you please explain further πŸ˜…?

Reload Message (Stamp of Quality Issue)

There is a tellraw that triggers on reload but it only shows the message to players with the tag x_LOAD_NOTIFY. This tag is never applied by the pack anywhere. It has to be manually applied by developers/testers if they wish to see the reload message. So unless the user specifically chooses to see reload messages by manually applying the tag to themselves, they won't see it.

There is also another tellraw message that triggers when users first install the pack (first-install, not reload). This message tells them to reload their world again, as the pack needs to load-in more functionality. The only way to avoid this message is to automatically reload instead of requesting the user to do it. However, calling the /reload function from within the datapack sometimes crashes minecraft!!

Global Ignoring Tag (Important Issue)

Are you saying not to modify user scores if they have the global.ignore tag? However, these are scores/objectives that are unique to my pack. Do I need to check for global.ignore tag before modifying my own scores? I have already checked for ignore tags in every command that modifies entity position, rotation, gui, etc. However, is that necessary for every score-check or score-modification too? Even if the scores are purely, only used in my pack?

Datapack Enabling (Bug Report)

I believe you're saying that the command datapack enable x_seat after "file / x_core" will cause an issue if the datapack filenames are different. If that is the case, can you suggest the right way to do it? My goal is to make sure that the S.E.A.T (x_seat) datapack only loads after the C.O.R.E (x_core) datapack. What is the right way to code that?

x-2mas commented 3 years ago

^ Bump ^

EstEarth202 commented 3 years ago

Certified Compatibility βœ”οΈ

Style Guideline Followed πŸ’•

Final comment: Cool pack, but I hope this pack will be optimized in the future.

If so, let me suggest, the easiest solution would be to put both. But this command will use the outermost file name. Therefore, it should be like this

datapack enable x_seat after "file/Xmas Labs - C.O.R.E" -> for folder
datapack enable x_seat after "file/Xmas Labs - C.O.R.E.zip" -> for .zip

You can now use the certified banner for this datapack. The link to your datapack will also be posted on the #certified-datapacks channel.

x-2mas commented 3 years ago

Thank you 😍! I'll make the change u suggested and update the pack soon!