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

feat: display items in hand of entities #293

Merged
merged 3 commits into from
Mar 7, 2025

Conversation

Phoenix616
Copy link

@Phoenix616 Phoenix616 commented Mar 4, 2025

User description

This adds the base functionality of displaying items in the hands of different entities. Currently supported ones:

  • Armor Stands
  • All Zombie types
  • Skeletons

Right now players are not supported as they do not use the inbuilt model format and the used library does not seem to have a concept of holding items. :/

In order to support Zombies the model was used from prismarine-viewer (and adjusted to fix the items) and the .obj version removed (which had the arms in a non-default position anyways. The "zombie stance" and other entity movements need to be done via some form of animation controller)also add missing left item elements to zombies


PR Type

Enhancement, Bug fix


Description

  • Added functionality to display items in the hands of entities.

    • Supported entities: Armor Stands, Zombies, Skeletons.
    • Introduced addItemModel method for handling item meshes.
  • Fixed left/right inversion issues for Armor Stands and other entities.

  • Replaced .obj models for Zombies with JSON-based models for better compatibility.

  • Adjusted scaling and positioning for items in different contexts (e.g., ground, third-person).


Changes walkthrough 📝

Relevant files
Enhancement
entities.ts
Enhance entity item handling and fix inversions                   

renderer/viewer/lib/entities.ts

  • Added addItemModel method to handle item meshes for entities.
  • Fixed left/right inversion for Armor Stands.
  • Adjusted scaling and positioning for items in various contexts.
  • Improved cleanup logic for entity meshes.
  • +66/-9   
    exportedModels.js
    Remove outdated Zombie and Husk models                                     

    renderer/viewer/lib/entity/exportedModels.js

    • Removed .obj models for Zombies and Husks.
    +0/-2     
    entities.json
    Add JSON models and improve animations                                     

    renderer/viewer/lib/entity/entities.json

  • Added JSON-based models for Zombies and Husks.
  • Fixed left/right item and limb pivots.
  • Enhanced animation controllers for various entity states.
  • +1074/-18
    zombie.obj
    Remove outdated Zombie `.obj` model                                           

    renderer/viewer/lib/entity/models/zombie.obj

    • Removed .obj model for Zombies.
    +0/-325 
    Bug fix
    index.ts
    Improve item UV retrieval logic                                                   

    src/index.ts

    • Updated getItemUv to handle missing itemId or blockId.
    +1/-1     
    armorModels.json
    Fix arm and leg naming and pivots                                               

    renderer/viewer/lib/entity/armorModels.json

  • Corrected left/right arm and leg naming.
  • Adjusted pivot points for better alignment.
  • +6/-6     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Summary by CodeRabbit

    • New Features

      • Introduced new mob entities with enhanced animations, textures, and spawn details.
    • Bug Fixes

      • Corrected limb misalignments and refined item attachment positions for improved visual consistency.
    • Refactor

      • Enhanced rendering logic with smoother scaling, rotation, and cleanup.
      • Improved item identification fallback to ensure reliable display in various contexts.

    Copy link

    codesandbox bot commented Mar 4, 2025

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    Copy link

    coderabbitai bot commented Mar 4, 2025

    Walkthrough

    This pull request updates the rendering and entity management logic. In the code, the scaling calculations and bone rotations are adjusted for different display contexts, and a new method for adding item models is introduced. The JSON files defining entity and armor models now have updated identifiers, including swapped bone names and revised item pivot values. New entities ("husk" and "zombie") are also added into the definitions while exports for these models have been removed from a separate module. Additionally, item identification is made more robust by using a fallback property in the item UV retrieval logic.

    Changes

    Files Change Summary
    renderer/viewer/lib/entities.ts Updated entity scaling logic based on display context, introduced new addItemModel method, enhanced cleanup, swapped bone rotations, and adjusted item frame mesh scaling/rotation.
    renderer/viewer/lib/entity/armorModels.json
    renderer/viewer/lib/entity/entities.json
    Renamed body part identifiers (arms and legs) and updated item pivot values; additionally, new entities ("husk" and "zombie") are added in entities.json.
    renderer/viewer/lib/entity/exportedModels.js Removed exports for zombie and husk models.
    src/index.ts Modified item UV retrieval logic to include a fallback using item.name when item.itemId and item.blockId are not available.

    Sequence Diagram(s)

    sequenceDiagram
        participant Caller
        participant Entities
        participant EntityMesh
        participant Bone
    
        Caller->>Entities: call addItemModel(entityMesh, hand, item)
        Entities->>EntityMesh: traverse to remove existing item in specified hand
        alt Item provided
            Entities->>Bone: retrieve appropriate bone for hand
            Entities->>Bone: attach retrieved item mesh
            Entities->>Bone: apply rotations and scaling (block vs. non-block)
        else No item provided
            Entities->>EntityMesh: finish without adding an item
        end
    
    Loading
    sequenceDiagram
        participant Caller
        participant getItemUv
        participant Item
        participant UVResolver
    
        Caller->>getItemUv: call getItemUv(item)
        getItemUv->>Item: check for item.itemId
        alt item.itemId exists
            getItemUv->>UVResolver: retrieve UV via itemId
        else
            getItemUv->>Item: check for item.blockId
            alt item.blockId exists
                getItemUv->>UVResolver: retrieve UV via blockId
            else
                getItemUv->>Item: use item.name as fallback
                getItemUv->>UVResolver: retrieve UV via item.name
            end
        end
        UVResolver-->>getItemUv: return UV coordinates
        getItemUv-->>Caller: return result
    
    Loading

    Possibly related PRs

    Suggested labels

    Review effort 3/5

    Poem

    I'm a little rabbit with a coder’s heart,
    Hopping through changes—each line a work of art.
    Scaling new heights and bones that swap in play,
    JSON and TS files guide my joyful day.
    With whiskers twitching at every fresh code beat,
    I celebrate our updates—with a hop and a treat!
    🐰💻

    ✨ Finishing Touches
    • 📝 Generate Docstrings

    🪧 Tips

    Chat

    There are 3 ways to chat with CodeRabbit:

    • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
      • I pushed a fix in commit <commit_id>, please review it.
      • Generate unit testing code for this file.
      • Open a follow-up GitHub issue for this discussion.
    • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
      • @coderabbitai generate unit testing code for this file.
      • @coderabbitai modularize this function.
    • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
      • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
      • @coderabbitai read src/utils.ts and generate unit testing code.
      • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
      • @coderabbitai help me debug CodeRabbit configuration file.

    Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

    CodeRabbit Commands (Invoked using PR comments)

    • @coderabbitai pause to pause the reviews on a PR.
    • @coderabbitai resume to resume the paused reviews.
    • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
    • @coderabbitai full review to do a full review from scratch and review all the files again.
    • @coderabbitai summary to regenerate the summary of the PR.
    • @coderabbitai generate docstrings to generate docstrings for this PR.
    • @coderabbitai resolve resolve all the CodeRabbit review comments.
    • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
    • @coderabbitai help to get help.

    Other keywords and placeholders

    • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
    • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
    • Add @coderabbitai anywhere in the PR title to generate the title automatically.

    CodeRabbit Configuration File (.coderabbit.yaml)

    • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
    • Please see the configuration documentation for more information.
    • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

    Documentation and Community

    • Visit our Documentation for detailed information on how to use CodeRabbit.
    • Join our Discord Community to get help, request features, and share feedback.
    • Follow us on X/Twitter for updates and announcements.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Memory Leak

    The additionalCleanup function may not properly dispose of all resources, particularly textures. The comment suggests textures are not being disposed which could lead to memory leaks.

    e.traverse(c => {
      if (c['additionalCleanup']) c['additionalCleanup']()
    })
    Scale Validation

    The SCALE variable is used without validation or bounds checking. Extreme scale values could cause rendering issues or performance problems.

    let SCALE = 1
    if (specificProps['minecraft:display_context'] === 'ground') {
      SCALE = 0.5
    } else if (specificProps['minecraft:display_context'] === 'thirdperson') {
      SCALE = 6
    }
    mesh.scale.set(SCALE, SCALE, SCALE)

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Standardize item pivot point values

    The item pivot points are inconsistent between different entity models. For
    example, zombie and husk use [-1, -45, -5] while other models use different
    values. Standardize the item pivot points across all entity models to ensure
    consistent item positioning.

    renderer/viewer/lib/entity/entities.json [772-773]

     {"name": "rightItem", "parent": "rightArm", "pivot": [-1, -45, -5]},
     {"name": "leftItem", "parent": "leftArm", "pivot": [1, -45, -5]},
    +// Apply these same pivot points consistently across all entity models
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Inconsistent item pivot points across different entity models can cause misaligned item rendering. The suggestion correctly identifies the need to standardize these values for consistent item positioning.

    Medium
    Limit maximum mesh scale factor

    The scale factor (SCALE) should have a maximum limit to prevent performance
    issues with extremely large meshes.

    renderer/viewer/lib/entities.ts [508-514]

     let SCALE = 1
     if (specificProps['minecraft:display_context'] === 'ground') {
       SCALE = 0.5
     } else if (specificProps['minecraft:display_context'] === 'thirdperson') {
    -  SCALE = 6
    +  SCALE = Math.min(6, 10) // Cap maximum scale
     }
     mesh.scale.set(SCALE, SCALE, SCALE)
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    __

    Why: While setting a maximum scale limit could prevent potential performance issues, the current scale of 6 is already reasonable and there's no evidence of performance problems in the current implementation.

    Low
    Possible issue
    Prevent texture disposal runtime errors

    The cleanup function for entity meshes should check if textures exist before
    attempting to dispose them to prevent potential runtime errors.

    renderer/viewer/lib/entities.ts [990-994]

     group['additionalCleanup'] = () => {
       // important: avoid texture memory leak and gpu slowdown
    -  itemObject.itemsTexture?.dispose()
    -  itemObject.itemsTextureFlipped?.dispose()
    +  if (itemObject.itemsTexture) itemObject.itemsTexture.dispose()
    +  if (itemObject.itemsTextureFlipped) itemObject.itemsTextureFlipped.dispose()
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: Adding explicit null checks before texture disposal is a good defensive programming practice that prevents potential runtime errors when textures are undefined, improving application stability.

    Medium
    • More

    Copy link

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 0

    🧹 Nitpick comments (2)
    renderer/viewer/lib/entities.ts (2)

    508-514: Avoid code duplication in scaling logic.

    The conditional scaling for 'ground' and 'thirdperson' is duplicated in multiple sections. Consider extracting it into a helper function to follow DRY principles.


    559-565: Repeated scaling logic.

    This block replicates the same scale checks as lines 508-514. Consolidating these conditions into a shared method would streamline maintenance and readability.

    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between 4b54be6 and fe86582.

    ⛔ Files ignored due to path filters (1)
    • renderer/viewer/lib/entity/models/zombie.obj is excluded by !**/*.obj
    📒 Files selected for processing (5)
    • renderer/viewer/lib/entities.ts (9 hunks)
    • renderer/viewer/lib/entity/armorModels.json (6 hunks)
    • renderer/viewer/lib/entity/entities.json (15 hunks)
    • renderer/viewer/lib/entity/exportedModels.js (0 hunks)
    • src/index.ts (1 hunks)
    💤 Files with no reviewable changes (1)
    • renderer/viewer/lib/entity/exportedModels.js
    ⏰ Context from checks skipped due to timeout of 90000ms (1)
    • GitHub Check: build-and-deploy
    🔇 Additional comments (19)
    renderer/viewer/lib/entity/armorModels.json (6)

    87-98: Naming and pivot check for the left arm segment.

    Renaming "rightarm" to "leftarm" matches the pivot shift from [ -5, 10, 0 ] to [ 5, 10, 0 ], ensuring correct mirroring. No issues found.


    100-112: Naming and pivot check for the right arm segment.

    Renaming "leftarm" to "rightarm" aligns properly with the pivot change. Looks consistent with the updated conventions.


    136-147: Naming the left leg segment.

    This block renames "rightleg" to "leftleg" alongside its pivot. The structure is coherent and adheres to the swapped naming approach.


    149-161: Renaming the right leg segment.

    Renaming "leftleg" to "rightleg" and reversing the pivot coordinates is consistent with the other swapped segments.


    172-183: Renaming the left leg segment in the feet section.

    This also reaffirms the naming swap for legs. No mismatch detected in pivot or UV usage.


    185-197: Renaming the right leg segment in the feet section.

    Renaming "leftleg" to "rightleg" is properly reflected in the pivot. No further modifications needed.

    src/index.ts (1)

    179-179: Fallback to item.name for better item identification.

    The expression item.itemId ?? item.blockId ?? item.name enhances robustness by using item.name if neither itemId nor blockId is present. Ensure item.name is always defined or handle the case where it may be missing.

    renderer/viewer/lib/entities.ts (2)

    717-718: Integrating held items for left and right hands.

    These calls to addItemModel(...) for each hand correctly delegate item rendering. No functional issues noted.


    972-1009: Implementation review of the new addItemModel method.

    1. The cleanup logic (additionalCleanup) correctly disposes textures, preventing memory leaks.
    2. Rotations and scales for blocks vs. standard items are well-differentiated.
    3. Code is modular and easy to extend if new contexts or transformations are needed.

    Overall, the method is well-structured and follows best practices.

    renderer/viewer/lib/entity/entities.json (10)

    44-45: Bone name swapping corrects model orientation for items.

    The arm bones have been renamed, swapping "rightarm" and "leftarm" to correct the model orientation. This is an important change to ensure items will display correctly in each hand according to Minecraft's coordinate system.

    Also applies to: 63-64


    52-52: Item pivot coordinates adjusted for proper item positioning.

    The pivot points for item bones have been significantly changed (from higher Y values to negative values with different offsets). These adjustments are essential for properly positioning held items in the entity's hands.

    Also applies to: 70-70


    72-73: Bone name swapping ensures consistency with arm changes.

    The leg bones have also been renamed to maintain consistency with the arm bone naming convention changes. This ensures a coherent model structure and consistent application of transformations.

    Also applies to: 74-77


    772-773: Item attachment points added for drowned entity.

    The drowned entity now has proper item bone definitions with correctly positioned pivot points. This enables the entity to hold items in both hands, which supports the PR objective of displaying items held by different entity types.


    1655-2182: New husk entity definition adds support for item display.

    A complete definition for the husk entity has been added with proper bone structure, animations, and item attachment points. This implementation is thorough and includes all necessary components for proper item display.


    3717-3719: Skeleton entity item pivot adjustment aligns with other entities.

    The skeleton entity's item pivot points have been adjusted to match the new standard positioning used across other entities. This consistency ensures items will be displayed correctly regardless of entity type.

    Also applies to: 3732-3734


    4542-4544: Stray entity item pivot adjustment maintains consistency.

    The stray entity (a skeleton variant) has the same item pivot adjustments, maintaining consistency across all skeleton-type entities. This approach ensures that rendering behavior is predictable across similar entity types.

    Also applies to: 4557-4559


    5577-6104: Comprehensive zombie entity implementation with proper item support.

    A complete zombie entity definition has been added with all necessary animations, controllers, and properly positioned item attachment points. The implementation is thorough and supports the display of items in both hands.


    6174-6175: Item attachment points for zombified piglin ensure consistent behavior.

    The zombified piglin entity has been updated with properly positioned item attachment points, ensuring consistent item display behavior across all zombie-type entities.

    Also applies to: 6189-6190


    3717-3719: Verify item placement across all supported entities in-game.

    While the coordinate adjustments appear consistent and well-implemented across all entity types, it would be beneficial to verify the actual in-game appearance to ensure items are properly positioned in all animation states (walking, attacking, etc.).

    Consider testing various item types (weapons, tools, blocks) with each supported entity to ensure proper positioning and orientation in all animation states.

    Also applies to: 3732-3734, 4542-4544, 4557-4559, 5630-5649, 5645-5649, 6174-6175, 6189-6190

    @zardoy
    Copy link
    Owner

    zardoy commented Mar 4, 2025

    add missing left item elements to zombies

    This is very cool, however since we support holding item display it makes so much sense to finish support for players, also will try to look into with a few days if you need help there.

    @zardoy
    Copy link
    Owner

    zardoy commented Mar 7, 2025

    so bad didnt find time to finish it:( hope to revisit in future

    @zardoy zardoy merged commit a7e6f97 into zardoy:next Mar 7, 2025
    3 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants