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

refactor: entity data acquisition and types #241

Merged
merged 18 commits into from
Feb 12, 2024

Conversation

leanmendoza
Copy link
Collaborator

@leanmendoza leanmendoza commented Feb 12, 2024

What?

Move core types from generic dictionaries to well-known types.

  • DclUserProfile
  • DclAvatarWireFormat
  • DclWearableEntityDefinition
  • DclSceneEntityDefinition

In the case DclUserProfile, methods from_godot_dictionary and to_godot_dictionary are remained for storing purporse

The previous step was done in the UserProfile but instead of being modified directly, a Dictionary wrapper was used in the GDScript code.

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2024

Codecov Report

Attention: 302 lines in your changes are missing coverage. Please review.

Comparison is base (72afc69) 47.85% compared to head (ac0f01d) 48.44%.
Report is 1 commits behind head on main.

Files Patch % Lines
.../decentraland-godot-lib/src/avatars/avatar_type.rs 52.38% 40 Missing ⚠️
.../decentraland-godot-lib/src/dcl/common/wearable.rs 49.27% 35 Missing ⚠️
...ust/decentraland-godot-lib/src/avatars/wearable.rs 78.52% 32 Missing ⚠️
...ust/decentraland-godot-lib/src/dcl/common/scene.rs 58.57% 29 Missing ⚠️
rust/decentraland-godot-lib/src/dcl/js/runtime.rs 0.00% 22 Missing ⚠️
...godot-lib/src/realm/dcl_scene_entity_definition.rs 71.01% 20 Missing ⚠️
...decentraland-godot-lib/src/avatars/avatar_scene.rs 9.52% 19 Missing ⚠️
...nd-godot-lib/src/realm/scene_entity_coordinator.rs 66.07% 19 Missing ⚠️
...ntraland-godot-lib/src/avatars/dcl_user_profile.rs 50.00% 18 Missing ⚠️
...aland-godot-lib/src/comms/communication_manager.rs 15.78% 16 Missing ⚠️
... and 13 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
+ Coverage   47.85%   48.44%   +0.59%     
==========================================
  Files         144      152       +8     
  Lines       17636    18089     +453     
==========================================
+ Hits         8439     8763     +324     
- Misses       9197     9326     +129     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leanmendoza leanmendoza marked this pull request as ready for review February 12, 2024 17:56
@kuruk-mm
Copy link
Member

kuruk-mm commented Feb 12, 2024

Can we use this PR to add the following base wearables? I can open another one anyway. But to avoid conflicts.

urn:decentraland:off-chain:base-avatars:black_glove
urn:decentraland:off-chain:base-avatars:cord_bracelet
urn:decentraland:off-chain:base-avatars:dcl_watch
urn:decentraland:off-chain:base-avatars:emerald_ring

@kuruk-mm
Copy link
Member

kuruk-mm commented Feb 12, 2024

Review:

chat.gd

		var avatar = Global.avatars.get_avatar_by_address(address)
		var avatar_name = (
			avatar.avatar_name if avatar != null else DclEther.shorten_eth_address(address)
		)

avatar.avatar_name doesn't exists

Repro steps: Go to Genesis Plaza and wait to receive a message

@kuruk-mm
Copy link
Member

kuruk-mm commented Feb 12, 2024

Review:

explorer.gd

		panel_chat.on_chats_arrived(
			[[player.avatar.avatar_id, player.avatar.avatar_name, 0, message]]
		)

avatar.avatar_name doesn't exists

Repro steps: Go to Genesis Plaza and try to talk

godot/src/test/avatar/spawn_and_move.gd Outdated Show resolved Hide resolved
rust/decentraland-godot-lib/src/avatars/godot_profile.rs Outdated Show resolved Hide resolved
}

#[func]
fn get_thumbnail(&self) -> GString {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn get_thumbnail(&self) -> GString {
fn get_thumbnail_url(&self) -> GString {

better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, I think is the hash, it directly holds the name of the property, I'd leave it

godot/src/decentraland_components/avatar.gd Show resolved Hide resolved
rust/decentraland-godot-lib/src/avatars/wearable.rs Outdated Show resolved Hide resolved
rust/decentraland-godot-lib/src/dcl/common/wearable.rs Outdated Show resolved Hide resolved
@leanmendoza leanmendoza requested a review from kuruk-mm February 12, 2024 22:58
Copy link
Member

@kuruk-mm kuruk-mm left a comment

Choose a reason for hiding this comment

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

Great!

rust/decentraland-godot-lib/src/dcl/common/wearable.rs Outdated Show resolved Hide resolved
@leanmendoza leanmendoza enabled auto-merge (squash) February 12, 2024 23:32
@leanmendoza leanmendoza merged commit 796259e into main Feb 12, 2024
3 checks passed
@leanmendoza leanmendoza deleted the refactor/entity-data-acquisition-and-types branch February 12, 2024 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants