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: option to disable ears on all skins #275

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

Phoenix616
Copy link

@Phoenix616 Phoenix616 commented Feb 13, 2025

User description

This adds an option to toggle whether or not the Deadmau5 ears are rendered for skins. Someone might want to toggle them off if skins include a full background there or information for some other mod. If toggled off the Vanilla logic of checking for Deadmau5's username is used and only their skin renders ears.


PR Type

Enhancement


Description

  • Added a configurable option to toggle Deadmau5 ears rendering.

  • Updated logic to conditionally render ears based on configuration.

  • Integrated the new renderEars option into the GUI and options storage.

  • Adjusted default settings and watchers to support the new feature.


Changes walkthrough 📝

Relevant files
Enhancement
entities.ts
Conditional rendering of Deadmau5 ears based on configuration

renderer/viewer/lib/entities.ts

  • Added logic to conditionally render Deadmau5 ears based on
    configuration.
  • Ensured ears rendering is disabled if the canvas is blank.
  • Refactored existing ears rendering logic to support new option.
  • +11/-6   
    optionsGuiScheme.tsx
    Integrated `renderEars` option into GUI scheme                     

    src/optionsGuiScheme.tsx

  • Added renderEars option to GUI scheme.
  • Provided tooltip description for renderEars option.
  • +3/-0     
    optionsStorage.ts
    Added `renderEars` to options storage                                       

    src/optionsStorage.ts

  • Added renderEars to default options storage.
  • Ensured renderEars is included in the options structure.
  • +1/-0     
    watchOptions.ts
    Updated watcher to handle `renderEars` configuration         

    src/watchOptions.ts

  • Updated watcher to apply renderEars configuration dynamically.
  • Ensured renderEars is synchronized with viewer configuration.
  • +1/-0     
    Configuration changes
    worldrendererCommon.ts
    Added `renderEars` to default configuration                           

    renderer/viewer/lib/worldrendererCommon.ts

  • Introduced renderEars as a default configuration option.
  • Set default value for renderEars to true.
  • +1/-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 13, 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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Memory Leak

    The earsCanvas element is created but never removed. Consider cleaning up this DOM element when it's no longer needed to prevent memory leaks.

    earsCanvas = document.createElement('canvas')
    loadEarsToCanvasFromSkin(earsCanvas, image)
    Error Handling

    The loadEarsToCanvasFromSkin function call has no error handling. If the skin image loading fails, it could cause runtime errors.

    loadEarsToCanvasFromSkin(earsCanvas, image)

    Copy link

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

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null check for safety

    The viewer configuration update should be wrapped in a null check to prevent
    potential runtime errors if the viewer object is undefined.

    src/watchOptions.ts [99]

    -viewer.world.config.renderEars = o.renderEars
    +if (viewer?.world) {
    +  viewer.world.config.renderEars = o.renderEars
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a potential runtime error by adding a null check for the viewer object, which is a critical safety improvement that could prevent application crashes in edge cases.

    Medium
    General
    Optimize canvas creation logic

    The renderEars condition should be evaluated after checking if the canvas is
    blank, not before. The current order creates an unnecessary canvas element when
    rendering is disabled.

    renderer/viewer/lib/entities.ts [353-359]

     let renderEars = this.viewer.world.config.renderEars || username === 'deadmau5'
    -let earsCanvas
    -if (renderEars) {
    -  earsCanvas = document.createElement('canvas')
    -  loadEarsToCanvasFromSkin(earsCanvas, image)
    -  renderEars = !isCanvasBlank(earsCanvas)
    -}
    +let earsCanvas = document.createElement('canvas')
    +loadEarsToCanvasFromSkin(earsCanvas, image)
    +renderEars = renderEars && !isCanvasBlank(earsCanvas)
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    __

    Why: While the suggestion would optimize the code by avoiding unnecessary canvas creation, the current implementation is still functional and the optimization gain would be minimal since this operation is not frequently executed.

    Low

    @zardoy
    Copy link
    Owner

    zardoy commented Feb 13, 2025

    conflicts with #261 have to finish that first. added one comment

    @zardoy
    Copy link
    Owner

    zardoy commented Feb 15, 2025

    oh as i thought now have conflicts:(

    # Conflicts:
    #	renderer/viewer/lib/entities.ts
    #	src/optionsGuiScheme.tsx
    #	src/watchOptions.ts
    @Phoenix616
    Copy link
    Author

    oh as i thought now have conflicts:(

    Resolved them :)

    @zardoy zardoy merged commit 874a3b3 into zardoy:next Feb 17, 2025
    2 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