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(sounds): Add sound variants and resource pack support! #258

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

zardoy
Copy link
Owner

@zardoy zardoy commented Jan 29, 2025

PR Type

Enhancement, Tests, Other


Description

  • Introduced a new sound system with sound variants and resource pack support.

  • Added scripts for uploading and managing sound files and maps.

  • Refactored resource pack handling for improved modularity.

  • Enhanced sound playback logic with dynamic sound mapping and resource pack integration.


Changes walkthrough 📝

Relevant files
Enhancement
8 files
rsbuild.config.ts
Added logic to copy generated sound files to distribution.
+3/-0     
uploadSoundFiles.ts
New script for uploading multiple sound files to GitHub. 
+109/-0 
uploadSounds.ts
Script for uploading individual sound files to GitHub.     
+67/-0   
resourcePack.ts
Refactored resource pack handling and added logging.         
+5/-4     
soundSystem.ts
Refactored sound system to use dynamic sound mapping.       
+40/-117
soundsMap.ts
Introduced `SoundMap` class for dynamic sound management.
+339/-0 
downloadSoundsMap.mjs
Updated sound map download script for new sound versions.
+1/-1     
prepareSounds.mjs
Enhanced sound preparation script with caching and modular actions.
+65/-27 
Tests
1 files
testSounds.ts
Added test script for sound map functionality.                     
+8/-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 Jan 29, 2025

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    @zardoy
    Copy link
    Owner Author

    zardoy commented Jan 29, 2025

    /deploy

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The script uploadSoundFiles.ts uses GitHub token directly in API requests without proper token scope validation. The token is exposed in request headers and could potentially be misused if intercepted. Consider implementing token scope validation and using environment variables securely.

    ⚡ Recommended focus areas for review

    Security Risk

    The script uses GitHub token for authentication but doesn't validate token permissions or scope. This could potentially allow unauthorized access if token is compromised.

    const token = process.env.GITHUB_TOKEN;
    
    // GitHub API endpoint
    const baseUrl = `https://api.github.com/repos/${owner}/${repo}/contents`;
    
    const headers = {
        Authorization: `token ${token}`,
        'Content-Type': 'application/json'
    };
    Possible Issue

    Sound playback may fail silently if soundMap is undefined. The error handling and fallback behavior should be improved.

    const playGeneralSound = async (soundKey: string, position?: Vec3, volume = 1, pitch?: number) => {
      if (!options.volume || !soundMap) return
      const soundData = await soundMap.getSoundUrl(soundKey, volume)
      if (!soundData) return
    
      const isMuted = options.mutedSounds.includes(soundKey) || options.volume === 0
    Performance

    The script downloads and processes sound files sequentially. Consider implementing parallel processing to improve performance.

    async function downloadSounds(version, assets, addPath = '') {
      if (addPath && existingSoundsCache.sounds[version]) {
        console.log('using existing sounds for version', version)
        return
      }
      console.log(version, 'have to download', assets.length, 'sounds')
      for (let i = 0; i < assets.length; i += 5) {
        await Promise.all(assets.slice(i, i + 5).map((asset, j) => downloadSound(asset, `${addPath}${asset.name}`, () => {
          console.log('downloading', addPath, asset.name, i + j, '/', assets.length)
        })))
      }

    Copy link

    qodo-merge-pro-for-open-source bot commented Jan 29, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add null check for bot.player
    Suggestion Impact:The commit includes a null check for bot.player before accessing bot.player.entity.velocity, though it's implemented slightly differently than suggested

    code diff:

    -    if (!bot.player || !soundMap) return // no info yet
    -    const VELOCITY_THRESHOLD = 0.1
    -    const { x, z, y } = bot.player.entity.velocity

    Add null check for bot.player before accessing bot.player.entity.velocity to prevent
    potential runtime errors.

    src/soundSystem.ts [92]

    +if (!bot.player?.entity) return
     const { x, z, y } = bot.player.entity.velocity
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical potential runtime error by adding a null check for bot.player and its entity property, which could prevent application crashes in edge cases.

    8
    Validate numeric weight values

    Validate the weight values before using them in calculations. The presence of
    debugger statements suggests potential NaN issues with weights.

    scripts/prepareSounds.mjs [201-206]

    -const minWeight = sounds.reduce((acc, cur) => cur.weight ? Math.min(acc, cur.weight) : acc, sounds[0].weight ?? 1)
    -if (isNaN(minWeight)) debugger
    +const weights = sounds.map(s => s.weight).filter(w => typeof w === 'number' && !isNaN(w));
    +const minWeight = weights.length > 0 ? Math.min(...weights) : 1;
     for (const sound of sounds) {
    -  if (sound.weight && isNaN(sound.weight)) debugger
    -  outputUseSoundLine.push(`${sound.volume ?? 1};${sound.name};${sound.weight ?? minWeight}`)
    +  const weight = (typeof sound.weight === 'number' && !isNaN(sound.weight)) ? sound.weight : minWeight;
    +  outputUseSoundLine.push(`${sound.volume ?? 1};${sound.name};${weight}`);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion effectively addresses potential runtime errors by properly validating weight values and removing debugger statements. This is critical as NaN values could cause calculation errors and affect the sound mapping functionality.

    8
    Add error handling for file reads

    Add error handling for failed file reads to prevent script from crashing if a file
    is inaccessible or corrupted.

    scripts/uploadSoundFiles.ts [49]

    -const content = fs.readFileSync(file.localPath, 'base64');
    +try {
    +  const content = fs.readFileSync(file.localPath, 'base64');
    +} catch (err) {
    +  console.error(`Failed to read file ${file.localPath}:`, err);
    +  throw err;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by adding try-catch for file reading operations, making the script more robust and providing better error reporting.

    7
    Add robust fetch error handling

    Add error handling for the fetch operations to handle network failures and invalid
    responses. Currently, multiple fetch calls are made without proper error handling.

    scripts/prepareSounds.mjs [166-167]

    -const burgerData = await fetch(burgerDataUrl(targetedVersions[0])).then((r) => r.json())
    -const { assetIndex } = await fetch(versionData.url).then((r) => r.json())
    -const index = await fetch(assetIndex.url).then((r) => r.json())
    +try {
    +  const burgerData = await fetch(burgerDataUrl(targetedVersions[0])).then(async (r) => {
    +    if (!r.ok) throw new Error(`Failed to fetch burger data: ${r.status}`);
    +    return r.json();
    +  });
    +  const { assetIndex } = await fetch(versionData.url).then(async (r) => {
    +    if (!r.ok) throw new Error(`Failed to fetch version data: ${r.status}`);
    +    return r.json();
    +  });
    +  const index = await fetch(assetIndex.url).then(async (r) => {
    +    if (!r.ok) throw new Error(`Failed to fetch asset index: ${r.status}`);
    +    return r.json();
    +  });
    +} catch (error) {
    +  console.error('Failed to fetch required data:', error);
    +  throw error;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses an important reliability issue by adding proper error handling for network requests. This would help diagnose and handle fetch failures more gracefully, improving the script's robustness.

    7

    Copy link

    Deployed to Vercel Preview: https://prismarine-kfthjg96w-zaro.vercel.app
    Playground
    Storybook

    @zardoy zardoy merged commit df44233 into next Jan 29, 2025
    3 checks passed
    @zardoy zardoy deleted the refactor-sounds branch January 29, 2025 01:54
    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.

    1 participant