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

fix bedrock entities format rendering (like arrow) #278

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

zardoy
Copy link
Owner

@zardoy zardoy commented Feb 18, 2025

PR Type

Enhancement, Tests


Description

  • Added support for per-face UV mapping in Bedrock entities.

  • Introduced validation for entity data using Zod schemas.

  • Improved error handling for missing or invalid UV coordinates.

  • Added a new test file for validating entity schemas.


Changes walkthrough 📝

Relevant files
Enhancement
EntityMesh.ts
Add per-face UV mapping and error handling                             

renderer/viewer/lib/entity/EntityMesh.ts

  • Added support for per-face UV mapping with CubeFaces.
  • Implemented getFaceUV function to handle UV retrieval.
  • Enhanced error handling for missing or invalid UV coordinates.
  • Adjusted UV calculations to support new and legacy formats.
  • +63/-8   
    Tests
    testEntities.ts
    Add Zod-based entity schema validation                                     

    renderer/viewer/lib/entity/testEntities.ts

  • Added Zod schemas for entity validation.
  • Validated entities.json against defined schemas.
  • Included error logging for schema validation failures.
  • Exported validated entities for further use.
  • +83/-0   

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

    codesandbox bot commented Feb 18, 2025

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The error handling for missing UV coordinates could be improved. Currently it just continues the loop on error, which could lead to incomplete or malformed meshes. Consider aggregating errors and handling them appropriately.

    if (!uvCoords) {
      errors.push(`Missing UV coordinates for face ${currentFace || 'unknown'}`)
      continue
    }
    Performance Concern

    The face direction lookup in the nested loops could be optimized. The current implementation searches through the faceToDirection mapping for each face iteration, which is inefficient.

    for (const [face, direction] of Object.entries(faceToDirection)) {
      if (direction === Object.keys(elemFaces)[Object.values(elemFaces).indexOf({ dir, corners, u0, v0, u1, v1 })]) {
        currentFace = face
        break
      }
    }
    Error Handling

    The schema validation error handling could be more robust. Currently it logs errors but then throws the original error, losing the formatted error messages. Consider creating a custom error type with the formatted messages.

    if (error instanceof z.ZodError) {
      console.error('Validation errors:')
      for (const err of error.errors) {
        console.error(`- Path: ${err.path.join('.')}`)
        console.error(`  Error: ${err.message}`)
      }
    }
    throw error

    Copy link

    qodo-merge-pro-for-open-source bot commented Feb 18, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Improve UV error handling

    The error handling for UV coordinates should fail fast and throw meaningful
    errors instead of silently continuing, as invalid UVs can cause rendering
    artifacts.

    renderer/viewer/lib/entity/EntityMesh.ts [220-223]

     if (!uvCoords) {
    -  errors.push(`Missing UV coordinates for face ${currentFace || 'unknown'}`)
    -  continue
    +  throw new Error(`Missing UV coordinates for face ${currentFace || 'unknown'} in cube at origin [${cube.origin.join(', ')}]`);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that silently continuing with invalid UV coordinates can lead to rendering issues. Throwing an error immediately with detailed cube information would help catch and debug UV mapping problems earlier.

    Medium
    General
    Optimize face direction lookup performance

    The face direction lookup in the loop is inefficient and error-prone. Extract
    the face direction mapping logic outside the loop and use a more direct mapping
    approach.

    renderer/viewer/lib/entity/EntityMesh.ts [207-212]

    -for (const [face, direction] of Object.entries(faceToDirection)) {
    -  if (direction === Object.keys(elemFaces)[Object.values(elemFaces).indexOf({ dir, corners, u0, v0, u1, v1 })]) {
    -    currentFace = face
    -    break
    -  }
    -}
    +const directionToFace = new Map([
    +  ['up', 'up'],
    +  ['down', 'down'],
    +  ['north', 'north'],
    +  ['south', 'south'],
    +  ['east', 'east'],
    +  ['west', 'west']
    +]);
    +currentFace = directionToFace.get(Object.keys(elemFaces)[Object.values(elemFaces).indexOf({ dir, corners, u0, v0, u1, v1 })]) as keyof CubeFaces;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The current implementation uses inefficient nested loops and complex object operations for face direction lookup. The suggested Map-based approach would significantly improve performance and code readability.

    Medium
    • Update

    @zardoy zardoy added the pr help needed You can continue working on this pr label Feb 18, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    pr help needed You can continue working on this pr Review effort 3/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant