Implemented Sign Editing Functionalities #262

whiteout314 · 2024-11-13T16:21:21Z

Description

Currently signs cannot be edited. Added the following:

Clientbound packet to open the screen editor when client places a sign block or interacts with a sign block.
Added serverbound packet that is sent after clicking done on the open screen editor window.
Added an interactive folder in blocks to hold interactive blocks such as signs, chests, etc...
Implemented a file that holds sign structure for NBT data and enums associated with valid sign blocks.
Added get_block_by_state_id function.
Fixed structure world's get_block function to search for state ids.

Testing

Tested using Minecraft version 1.21.3 placing signs and tested placing sign and able to see the sign editor and tested as well that an already placed sign can modified (front and back).

Checklist

Things need to be done before this Pull Request can be merged.

Code is well-formatted and adheres to project style guidelines: cargo fmt
Code does not produce any clippy warnings cargo clippy

Snowiiii · 2024-11-13T21:01:10Z

pumpkin-world/src/block/interactive/sign.rs

+pub enum SignType {
+    OakSign = 194,
+    SpruceSign = 195,
+    BirchSign = 196,
+    AcaciaSign = 197,
+    CherrySign = 198,
+    JungleSign = 199,
+    DarkOakSign = 200,
+    PaleOakSign = 201,
+    MangroveSign = 202,
+    BambooSign = 203,
+
+    OakWallSign = 208,
+    SpruceWallSign = 209,
+    BirchWallSign = 210,
+    AcaciaWallSign = 211,
+    CherryWallSign = 212,
+    JungleWallSign = 213,
+    DarkOakWallSign = 214,
+    PaleOakWallSign = 215,
+    MangroveWallSign = 216,
+    BambooWallSign = 217,
+
+    OakHangingSign = 218,
+    SpruceHangingSign = 219,
+    BirchHangingSign = 220,
+    AcaciaHangingSign = 221,
+    CherryHangingSign = 222,
+    JungleHangingSign = 223,
+    DarkOakHangingSign = 224,
+    PaleOakHangingSign = 225,
+    CrimsonHangingSign = 226,
+    WarpedHangingSign = 227,
+    MangroveHangingSign = 228,
+    BambooHangingSign = 229,
+
+    OakWallHangingSign = 230,
+    SprucWalleHangingSign = 231,
+    BirchWallHangingSign = 232,
+    AcaciaWallHangingSign = 233,
+    CherryWallHangingSign = 234,
+    JungleWallHangingSign = 235,
+    DarkOakWallHangingSign = 236,
+    PaleOakWallHangingSign = 237,
+    CrimsonWallHangingSign = 238,
+    WarpedWallHangingSign = 239,
+    MangroveWallHangingSign = 240,
+    BambooWallHangingSign = 241,
+
+    CrimsonSign = 849,
+    WarpedSign = 850,
+    CrimsonWallSign = 851,
+    WarpedWallSign = 852,
+}


This looks horrible, Also the the ID's will change with every Update

Now looking at it again, it is. I will make sure to fix this.

Added a macro that looks up the block entity id instead of the actual id and eliminated the mess.

Snowiiii · 2024-11-13T21:03:32Z

pumpkin/src/entity/player.rs

@@ -871,6 +873,9 @@ impl Player {
            SSwingArm::PACKET_ID => {
                self.handle_swing_arm(SSwingArm::read(bytebuf)?).await;
            }
+            SUpdateSign::PACKET_ID => {
+                //TODO


So this is empty, So Im predicting that Signs currently do not work right ?.
I recommend to make this a draft PR then when its still WIP

In the game the sign updates with the text. I believe that this packet is suppose to update the server's nbt file--from what I have been reading. Couldn't test to see if another client sees the modified sign since I don't have another account.

Figured it out, my bad. My latest commit implements the function that changes the sign for all clients by sending a client bound Block Entity Data packet.

whiteout314 · 2024-11-14T04:34:51Z

Snowiiii · 2024-11-14T15:42:25Z

pumpkin-macros/src/block_entity.rs

+static BLOCK_ENTITIES: LazyLock<HashMap<String, u32>> = LazyLock::new(|| {
+    serde_json::from_str::<TopLevel>(include_str!("../../assets/blocks.json"))
+        .expect("Could not parse blocks.json registry.")
+        .block_entity_types
+        .into_iter()
+        .map(|value| (value.ident, value.id))
+        .collect()
+});


Now blocks.json is loaded twice. Here and in pumpkin-world. We want to avoid this, Its an Performance and Resource Waste. Most JSON's are currently being loaded from pumpkin-macros. We free to move some methods from the block_registry to here

I also would love to have the BlockState::new(<name>).unwrap() replaced with a macro. We had a macro for this before i moved the JSON's to our own Extractor. Feel free to add this to this PR, But you not forced

Snowiiii · 2024-11-14T15:50:20Z

pumpkin-protocol/src/server/play/s_update_sign.rs

+    pub line_1: String,
+    pub line_2: String,
+    pub line_3: String,
+    pub line_4: String,


Every line has the max length of 384. We currently have no way of automatically Deserializing the length. You would need to have to implement the Packet manually and use buf.read_string_len(384)

If it's 384 characters per line, then why not

Suggested change

pub line_1: String,

pub line_2: String,

pub line_3: String,

pub line_4: String,

pub line_1: [char; 384],

pub line_2: [char; 384],

pub line_3: [char; 384],

pub line_4: [char; 384],

?

I forgot that the client is probably going to try and send something more than 384 characters and crash the server... 💀

If it's 384 characters per line, then why not

?

That's the wrong approche, It will be also a pain to actually get the lines.

If it's 384 characters per line, then why not
?

That's the wrong approche, It will be also a pain to actually get the lines.

You do save some memory by storing it as a slice though. Also, what do you mean "get the lines"? If you're using it like it's a String then you just use String::from_slice or line_n.into(). Although I do think that we probably shouldn't trust that the client is going to send 384 characters or below for a line.

leobeg · 2024-11-16T18:17:54Z

pumpkin-protocol/src/server/play/s_update_sign.rs

+        let location = WorldPosition(Vector3::new(
+            bytebuf.get_i32()?,
+            bytebuf.get_i32()?,
+            bytebuf.get_i32()?,


This won't work. Look at https://wiki.vg/Protocol#Position. My solution:

pumpkin-core/src/math/position impl WorldPosition add

pub fn from_i64(encoded_position: i64) -> Self { WorldPosition(Vector3 { x: (encoded_position >> 38) as i32, y: (encoded_position << 52 >> 52) as i32, z: (encoded_position << 26 >> 38) as i32, }) }

And in your SUpdateSign you just do:

let location = WorldPosition::from_i64(bytebuf.get_i64()?);

Added this, thanks for the solution!

leobeg · 2024-11-16T18:46:24Z

There is also an error when the block gets updatet trough the CAcknowledgeBlockChange packet. Then the popup instantly goes away

leobeg · 2024-11-17T14:32:58Z

@whiteout314 why was this PR closed?

DataM0del · 2024-11-17T14:45:50Z

oof

whiteout314 · 2024-11-17T15:07:01Z

Sorry about that looks like I had an issue last night updating and pushing to my branch

whiteout314 · 2024-11-17T15:10:10Z

pumpkin-macros/src/block.rs

+    let block_entity_name = input_string.trim_matches('"');
+
+    quote! {
+        pumpkin_world::block::block_registry::BLOCKS


@Snowiiii I was wondering if this is fine and in another PR, I could do all the changes to bring the methods from block_registry into this macro

pumpkin/src/client/player_packet.rs

leobeg · 2024-11-22T18:54:21Z

Will this be merged soon? It would be a base for following block entities.

OfficialKris · 2024-11-22T20:02:27Z

Block entities are also required for many things. I wanted to add jukebox support and even that requires it (storing disc). Will this be merged soon? I tested and it works for me.

leobeg · 2024-11-25T20:41:37Z

I think it would be best to fit this PR to the new Block system in #333

leobeg · 2024-11-29T09:12:25Z

@whiteout314 Do you want to do this or should I implement this?

whiteout314 · 2024-11-29T15:11:58Z

@whiteout314 Do you want to do this or should I implement this?

@leobeg Go ahead and implement it since you are already working on blocks that can be interacted with and plus is up to date.

Snowiiii · 2024-12-07T12:11:51Z

@whiteout314 #333 just got merged

Snowiiii reviewed Nov 13, 2024

View reviewed changes

whiteout314 marked this pull request as draft November 13, 2024 23:53

whiteout314 marked this pull request as ready for review November 14, 2024 04:35

Snowiiii reviewed Nov 14, 2024

View reviewed changes

Snowiiii mentioned this pull request Nov 16, 2024

Add sign support #296

Closed

2 tasks

leobeg reviewed Nov 16, 2024

View reviewed changes

whiteout314 added 4 commits November 17, 2024 00:30

Updated revised areas

da9a1ad

Removed enum added macro to search for block entity id

5139085

Added sign update for all clients & clean up

54a49b9

updated update_sign

88fed04

whiteout314 closed this Nov 17, 2024

whiteout314 force-pushed the whiteout314-dev-branch branch from 1662038 to 00af2b9 Compare November 17, 2024 05:34

whiteout314 reopened this Nov 17, 2024

whiteout314 closed this Nov 17, 2024

Updated code

62d4059

whiteout314 reopened this Nov 17, 2024

whiteout314 commented Nov 17, 2024

View reviewed changes

pumpkin/src/client/player_packet.rs Outdated Show resolved Hide resolved

typo fix and format

9f28a54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Sign Editing Functionalities #262

Implemented Sign Editing Functionalities #262

whiteout314 commented Nov 13, 2024

Snowiiii Nov 13, 2024

whiteout314 Nov 13, 2024 •

edited

Loading

whiteout314 Nov 14, 2024

Snowiiii Nov 13, 2024

whiteout314 Nov 13, 2024

whiteout314 Nov 14, 2024

whiteout314 commented Nov 14, 2024

Snowiiii Nov 14, 2024

Snowiiii Nov 14, 2024

Snowiiii Nov 14, 2024 •

edited

Loading

DataM0del Nov 14, 2024 •

edited

Loading

DataM0del Nov 14, 2024

Snowiiii Nov 14, 2024

DataM0del Nov 14, 2024

leobeg Nov 16, 2024

whiteout314 Nov 17, 2024

leobeg commented Nov 16, 2024

leobeg commented Nov 17, 2024

DataM0del commented Nov 17, 2024

whiteout314 commented Nov 17, 2024

whiteout314 Nov 17, 2024

leobeg commented Nov 22, 2024

OfficialKris commented Nov 22, 2024

leobeg commented Nov 25, 2024

leobeg commented Nov 29, 2024

whiteout314 commented Nov 29, 2024

Snowiiii commented Dec 7, 2024

Implemented Sign Editing Functionalities #262

Are you sure you want to change the base?

Implemented Sign Editing Functionalities #262

Conversation

whiteout314 commented Nov 13, 2024

Description

Testing

Checklist

Choose a reason for hiding this comment

whiteout314 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whiteout314 commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snowiiii Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

DataM0del Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobeg commented Nov 16, 2024

leobeg commented Nov 17, 2024

DataM0del commented Nov 17, 2024

whiteout314 commented Nov 17, 2024

Choose a reason for hiding this comment

leobeg commented Nov 22, 2024

OfficialKris commented Nov 22, 2024

leobeg commented Nov 25, 2024

leobeg commented Nov 29, 2024

whiteout314 commented Nov 29, 2024

Snowiiii commented Dec 7, 2024

whiteout314 Nov 13, 2024 •

edited

Loading

Snowiiii Nov 14, 2024 •

edited

Loading

DataM0del Nov 14, 2024 •

edited

Loading