impl enums for v662 #68

OmniacDev · 2024-11-21T13:47:33Z

No description provided.

theaddonn · 2024-11-21T14:24:50Z

Something I noticed in general is that you are creating a new module/namespace quite often, that is unnecessary..
for example actor_block_sync_message::ActorBlockSyncMessage::MessageId gets quite long, Rust already creates a new module for every file, so you might want to remove the additional module in the file and rename the internal enum to something like ActorBlockSyncMessageId

theaddonn · 2024-11-21T14:41:39Z

Also something to avoid in the future (only avoid this for stuff in the /enums and /types directory), having the word packet in the file/type name.. this may easily confuse people with the actual file for the packet :)

theaddonn · 2024-11-21T15:32:19Z

Also, it is not that important to match Mojang's names 1:1 since they can be quite confusing at times

theaddonn

So urm, this review got quite big....

theaddonn · 2024-11-21T14:25:43Z

crates/proto/src/version/v662/enums/actor_block_sync_message.rs

@@ -0,0 +1,10 @@
+pub mod ActorBlockSyncMessage {


No need for the additional mod, you can just rename the type to ActorBlockSyncMessageId :)

theaddonn · 2024-11-21T14:26:48Z

crates/proto/src/version/v662/enums/animate_packet.rs

@@ -0,0 +1,14 @@
+pub mod AnimatePacket {


No need for the additional mod, you can just rename the type to AnimateAction :)

theaddonn · 2024-11-21T14:27:27Z

crates/proto/src/version/v662/enums/animate_packet.rs

@@ -0,0 +1,14 @@
+pub mod AnimatePacket {


Also, it is confusing when you add an extra packet in the file name, maybe change it to crates/proto/src/version/v662/enums/animate.rs

crates/proto/src/version/v662/enums/build_platform.rs

theaddonn · 2024-11-21T15:53:50Z

crates/proto/src/version/v662/enums/position_tracking_db_client_request_packet.rs

@@ -0,0 +1,8 @@
+pub mod PositionTrackingDBClientRequestPacket {


It might be a good idea to remove the extra mod and just call the enum PositionTrackingDBClientRequestAction

theaddonn · 2024-11-21T15:57:08Z

crates/proto/src/version/v662/enums/position_tracking_db_server_broadcast_packet.rs

@@ -0,0 +1,10 @@
+pub mod PositionTracingDBServerBroadcastPacket {


It might be a good idea to remove the extra mod and just call the enum PositionTracingDBServerBroadcastAction

theaddonn · 2024-11-21T16:02:26Z

crates/proto/src/version/v662/enums/position_tracking_db_server_broadcast_packet.rs

@@ -0,0 +1,10 @@
+pub mod PositionTracingDBServerBroadcastPacket {


It may make sense to also merge this with position_tracking_db_client_request_packet.rs into one file called position_tracking_db.rs

theaddonn · 2024-11-21T16:07:45Z

crates/proto/src/version/v662/enums/minecraft_eventing.rs

@@ -0,0 +1,63 @@
+pub mod MinecraftEventing {


I don't even know what to say here, this is a true mess and idk what happened

Just maybe remove the extra mod and get each enum a separate file.. I guess

theaddonn · 2024-11-21T16:09:42Z

crates/proto/src/version/v662/enums/puv.rs

@@ -0,0 +1,460 @@
+pub mod Puv {


Seems like quite a lot of module nesting... why dont you just call the enum LegacyLevelSoundEvent.. adjust the file name and remove the other mods?

OmniacDev · 2024-11-21T22:21:50Z

Something I noticed in general is that you are creating a new module/namespace quite often, that is unnecessary..
for example actor_block_sync_message::ActorBlockSyncMessage::MessageId gets quite long, Rust already creates a new module for every file, so you might want to remove the additional module in the file and rename the internal enum to something like ActorBlockSyncMessageId

Yeah this was sort of temporary until I can find the other stuff in the namespace, types& packets etc. I added comments in the mod.rs file for all the modules in there so I can easily go back and change them later

OmniacDev · 2024-11-21T23:23:23Z

these workflows are annoying lmao, spamming my email every time I push

OmniacDev · 2024-11-22T01:59:29Z

@theaddonn I might leave this as is for now, planning on coming back later after implementing all the types and packets to clean everything up.

theaddonn · 2024-11-22T08:54:32Z

Sounds good!

OmniacDev and others added 30 commits November 5, 2024 20:35

[enum] abilities_index.rs

44c6737

[enum] actor_block_sync_message.rs

f359400

[enum] actor_damage_cause.rs

47f4d56

[enum] actor_data_ids.rs

6c7cbdf

[enum] actor_event.rs

adb4791

[enum] actor_flags.rs

888b45c

[enum] actor_link_type.rs

c0148e8

[enum] actor_type.rs

26aa546

[enum] agent_action_type.rs

67fb711

[enum] animate_packet.rs (MOD)

5d909f3

[enum] animation_mode.rs

dde182c

[enum] attribute_modifier_operation.rs

76fea36

[enum] attribute_operands.rs

abbe599

[enum] book_edit_action.rs

9b355de

[enum] boss_event_update_type.rs

fcdf775

[enum] build_platform.rs

5e980ab

[enum] camera_preset.rs (MOD)

8dc93eb

[enum] camera_shake_action.rs

9d2ce01

[enum] camera_shake_type.rs

43b5703

[enum] chat_restriction_level.rs

f191f01

[enum] client_play_mode.rs

cd19f79

[enum] clientbound_debug_renderer_packet.rs (MOD)

737ff76

[enum] clientbound_map_item_data_packet.rs (MOD)

6d11e87

[enum] code_builder_storage_query_options.rs (MOD)

7c2d2f0

[enum] command_block_mode.rs

5901f18

Merge branch 'bedrock-crustaceans:proto_rework' into proto_rework

ea9ff0e

[enum] command_origin_type.rs

931876a

[enum] command_output_type.rs

a39e6d6

[enum] command_parameter_option.rs

73c275a

[enum] command_permission_level.rs

d671d03

OmniacDev added 18 commits November 22, 2024 02:21

[enum] simulation_type.rs

6390196

[enum] social.rs (MOD)

17ece9e

[enum] soft_enum_update_type.rs

aa5f0e7

[enum] spawn_biome_type.rs

7221ae7

[enum] spawn_position_type.rs

e86c720

[enum] structure_block_type.rs

ebe5cc0

[enum] structure_redstone_save_mode.rs

28c1b8c

[enum] inventory_source_type.rs

45ee9d6

[enum] structure_template_request_operation.rs

7ef10f0

[enum] structure_template_response_type.rs

e3606fd

[enum] sub_chunk_packet.rs (MOD)

8110c2c

[enum] tag.rs (MOD)

9f7fdbd

[enum] text_packet_type.rs

43ac7e2

[enum] text_processing_event_origin.rs

ceb0778

[enum] ui_profile.rs

419226e

[enum] persona.rs (MOD)

48cedda

[enum] mod.rs

ae75c64

[enum] mod.rs

7a66043

theaddonn requested changes Nov 21, 2024

View reviewed changes

OmniacDev added 3 commits November 22, 2024 12:08

[enum] minecraft_packet_ids.rs

76c7f66

add #[deprecated] tags

48572f0

[enum] position_tracking_db_server_broadcast_packet.rs

2976365

theaddonn merged commit 3e258f1 into bedrock-crustaceans:proto_rework Nov 23, 2024
2 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl enums for v662 #68

impl enums for v662 #68

OmniacDev commented Nov 21, 2024

theaddonn commented Nov 21, 2024

theaddonn commented Nov 21, 2024

theaddonn commented Nov 21, 2024

theaddonn left a comment

theaddonn Nov 21, 2024

theaddonn Nov 21, 2024

theaddonn Nov 21, 2024

theaddonn Nov 21, 2024

theaddonn Nov 21, 2024

theaddonn Nov 21, 2024

theaddonn Nov 21, 2024

theaddonn Nov 21, 2024

OmniacDev commented Nov 21, 2024

OmniacDev commented Nov 21, 2024

OmniacDev commented Nov 22, 2024

theaddonn commented Nov 22, 2024

		@@ -0,0 +1,8 @@
		pub mod PositionTrackingDBClientRequestPacket {

		@@ -0,0 +1,10 @@
		pub mod PositionTracingDBServerBroadcastPacket {

impl enums for v662 #68

impl enums for v662 #68

Conversation

OmniacDev commented Nov 21, 2024

theaddonn commented Nov 21, 2024

theaddonn commented Nov 21, 2024

theaddonn commented Nov 21, 2024

theaddonn left a 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

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

OmniacDev commented Nov 21, 2024

OmniacDev commented Nov 21, 2024

OmniacDev commented Nov 22, 2024

theaddonn commented Nov 22, 2024