Conversation
- absolument pas optimisé - carrément explosé - mais c'est dur à faire - donc laissez moi trqnl
|
Warning Rate limit exceeded@Desoroxxx has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to several files in a Minecraft mod project, primarily focusing on sound handling functionalities. Key changes include the addition of new classes and message handlers for realistic sound effects, updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (8)
src/main/java/com/paneedah/mwc/network/handlers/RealisticSoundHandler.java (1)
26-32: Avoid unnecessary task scheduling inside an already scheduled taskYou are scheduling a new task inside an existing scheduled task, which is unnecessary since the outer task runs on the main server thread. This can be simplified by sending the message directly within the current task.
Apply this diff to simplify the code:
- playerMP.getServerWorld().addScheduledTask(() -> { CHANNEL.sendTo( new RealisticSoundClientMessage(sound.getSound(), message.getPos(), sound.getVolumeIn(), sound.getPitchIn(), distance), playerMP ); - });src/main/java/com/paneedah/weaponlib/sound/ClientSoundPlayer.java (1)
13-13: Method name should follow camelCase conventionThe method
PlaySoundClientshould be renamed toplaySoundClientto adhere to Java naming conventions for method names.Apply this diff to rename the method:
- public void PlaySoundClient(double distance, SoundEvent soundIn, SoundCategory categoryIn, float volumeIn, float pitchIn, float xIn, float yIn, float zIn) { + public void playSoundClient(double distance, SoundEvent soundIn, SoundCategory categoryIn, float volumeIn, float pitchIn, float xIn, float yIn, float zIn) {🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (next)
[warning] 13-23: ❌ New issue: Excess Number of Function Arguments
PlaySoundClient has 8 arguments, threshold = 4. This function has too many arguments, indicating a lack of encapsulation. Avoid adding more arguments.src/main/java/com/paneedah/weaponlib/sound/RealisticSound.java (1)
28-36: Consider enhancing volume attenuation for realismThe current linear attenuation model may not accurately reflect how sound diminishes over distance in reality. Consider using an inverse square law or exponential attenuation for more realistic sound fading.
Modify the
adjustVolumeForDistancemethod to use an exponential decay:- return 1.0 - (distance / maxDistance); + return Math.exp(-distance / maxDistance);This change will make distant sounds fade more realistically.
src/main/java/com/paneedah/weaponlib/WeaponFireAspect.java (5)
289-289: Consider extracting world type ID to a constant.The world type ID is used directly in the message. Consider extracting it to a named constant for better maintainability.
+ private static final String WORLD_TYPE_ID = "world_type_id"; - CHANNEL.sendToServer(new RealisticSoundMessage(silencerOn, shootSound, player.getPosition(), player.getEntityWorld().getWorldType().getId())); + CHANNEL.sendToServer(new RealisticSoundMessage(silencerOn, shootSound, player.getPosition(), WORLD_TYPE_ID));🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (next)
[notice] 289-368: ✅ Getting better: Complex Method
fire decreases in cyclomatic complexity from 26 to 25, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
300-303: Consider extracting sound message sending logic to a helper method.The sound message sending logic is duplicated between regular shots and end-of-shoot sounds. Consider extracting it to a helper method.
+ private void sendSoundMessage(EntityLivingBase player, boolean silencerOn, SoundEvent sound) { + if (!FMLCommonHandler.instance().getSide().isServer()) { + CHANNEL.sendToServer(new RealisticSoundMessage(silencerOn, sound, + player.getPosition(), player.getEntityWorld().getWorldType().getId())); + } + } - if(currentAmmo == 1 && weapon.getEndOfShootSound() != null && !FMLCommonHandler.instance().getSide().isServer()) { - CHANNEL.sendToServer(new RealisticSoundMessage(silencerOn,weapon.getEndOfShootSound(), - player.getPosition(), player.getEntityWorld().getWorldType().getId())); + if(currentAmmo == 1 && weapon.getEndOfShootSound() != null) { + sendSoundMessage(player, silencerOn, weapon.getEndOfShootSound());
Line range hint
353-372: Remove commented out code.Large blocks of commented code should be removed as they add unnecessary noise to the codebase. If this code is needed for reference, it should be documented in the commit history or documentation.
Line range hint
423-434: Remove commented out code block.This block of commented code should be removed. If the functionality is needed in the future, it can be retrieved from version control.
Line range hint
289-303: Sound system architecture looks good.The transition from client-side to server-side sound handling with distance-based playback is a good architectural choice. It allows for:
- Centralized sound management
- Better synchronization across clients
- More realistic sound propagation based on distance
Consider documenting the sound propagation mechanics (distance calculations, delay factors) in the class-level documentation for future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
build.gradle.kts(1 hunks)src/main/java/com/paneedah/mwc/MWC.java(2 hunks)src/main/java/com/paneedah/mwc/network/handlers/RealisticSoundClientHandler.java(1 hunks)src/main/java/com/paneedah/mwc/network/handlers/RealisticSoundHandler.java(1 hunks)src/main/java/com/paneedah/mwc/network/messages/RealisticSoundClientMessage.java(1 hunks)src/main/java/com/paneedah/mwc/network/messages/RealisticSoundMessage.java(1 hunks)src/main/java/com/paneedah/weaponlib/WeaponFireAspect.java(8 hunks)src/main/java/com/paneedah/weaponlib/sound/ClientSoundPlayer.java(1 hunks)src/main/java/com/paneedah/weaponlib/sound/RealisticSound.java(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (next)
src/main/java/com/paneedah/weaponlib/WeaponFireAspect.java
[notice] 289-368: ✅ Getting better: Complex Method
fire decreases in cyclomatic complexity from 26 to 25, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[notice] 289-368: ✅ Getting better: Complex Method
fire decreases in cyclomatic complexity from 26 to 25, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
src/main/java/com/paneedah/weaponlib/sound/ClientSoundPlayer.java
[warning] 13-23: ❌ New issue: Excess Number of Function Arguments
PlaySoundClient has 8 arguments, threshold = 4. This function has too many arguments, indicating a lack of encapsulation. Avoid adding more arguments.
🔇 Additional comments (4)
src/main/java/com/paneedah/mwc/network/messages/RealisticSoundClientMessage.java (1)
13-22: LGTM! Class structure is well-organized.
The use of Lombok annotations effectively reduces boilerplate code. The fields are appropriately chosen for sound message handling.
build.gradle.kts (1)
26-26: LGTM! Configuration update.
The username change is straightforward and doesn't impact functionality.
src/main/java/com/paneedah/mwc/MWC.java (1)
126-126: LGTM! Message handlers are properly registered.
The registration of RealisticSound handlers follows the established pattern:
- Client handler with negative ID (-17)
- Server handler with positive ID (14)
Let's verify there are no ID conflicts:
Also applies to: 141-141
✅ Verification successful
Message handler IDs are unique and properly segregated
The verification confirms that:
- All client-side handlers use negative IDs (-1 to -17)
- All server-side handlers use positive IDs (1 to 14)
- There are no duplicate IDs
- The RealisticSound handlers follow this pattern with -17 (client) and 14 (server)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate message handler IDs
# Expect: No duplicate IDs in the output
rg -A 1 'CHANNEL\.registerMessage.*class,\s*[+-]?\d+,\s*Side\.' | grep -o '[+-]\d\+' | sort -n | uniq -d
Length of output: 106
Script:
#!/bin/bash
# Let's try a more flexible pattern to find message registrations
rg -A 1 'CHANNEL\.registerMessage.*Side\.' --no-heading
Length of output: 4953
src/main/java/com/paneedah/weaponlib/WeaponFireAspect.java (1)
7-7: LGTM: Import addition aligns with sound enhancement objectives.
| double distance = Math.sqrt(message.getPos().distanceSq(playerInWorld.getPosition())); | ||
| RealisticSound sound = RealisticSound.createSound(message.getSilencer(), message.getSound(), message.getPos(), playerInWorld.getPosition()); | ||
| if (sound.getVolumeIn() > 0.0F) { | ||
| EntityPlayerMP playerMP = (EntityPlayerMP) playerInWorld; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/paneedah/mwc/network/handlers/RealisticSoundHandler.java (1)
25-26: Add null checks and optimize distance calculation.The current implementation lacks null checks and uses a potentially expensive square root operation.
Consider these improvements:
- double distance = Math.sqrt(message.getPos().distanceSq(playerInWorld.getPosition())); - RealisticSound sound = RealisticSound.createSound(message.getSilencer(), message.getSound(), message.getPos(), playerInWorld.getPosition()); + if (message.getPos() != null && playerInWorld.getPosition() != null) { + // Use distanceSq for volume checks to avoid sqrt + double distanceSq = message.getPos().distanceSq(playerInWorld.getPosition()); + // Only calculate actual distance if needed for sound delay + double distance = Math.sqrt(distanceSq); + RealisticSound sound = RealisticSound.createSound(message.getSilencer(), message.getSound(), message.getPos(), playerInWorld.getPosition());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/com/paneedah/mwc/MWC.java(2 hunks)src/main/java/com/paneedah/mwc/network/handlers/RealisticSoundHandler.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/paneedah/mwc/MWC.java
🔇 Additional comments (4)
src/main/java/com/paneedah/mwc/network/handlers/RealisticSoundHandler.java (4)
1-16: LGTM! Imports are clean and appropriate.
All necessary imports are present and properly organized.
17-17: LGTM! Class declaration follows Forge's networking patterns.
The class properly implements IMessageHandler with correct generic type parameters.
28-36: Previous instance check implementation is correct.
The code already implements the safe casting pattern that was suggested in the previous review.
31-34: Verify sound distance limits.
According to the PR objectives, weapons with silencers should be heard up to 100 blocks away, while those without can be heard up to 400 blocks.
Let's verify the distance limits in the codebase:
✅ Verification successful
Based on the search results, I found the relevant distance limits defined in RealisticSound.java:
private static final double MAX_DISTANCE = 400D;
private static final double MAX_DISTANCE_SILENCER = 100D;The code shows that the distance limits are correctly implemented:
- Weapons with silencers have a max sound distance of 100 blocks
- Weapons without silencers have a max sound distance of 400 blocks
The volume is adjusted based on these limits using the adjustVolumeForDistance() method, which scales the volume linearly from 1.0 at distance 0 to 0.0 at the max distance.
The distance limits match the PR objectives exactly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for distance-related constants in RealisticSound class
ast-grep --pattern 'class RealisticSound {
$$$
private static final $_ MAX_DISTANCE = $_;
$$$
}'
Length of output: 104
Script:
#!/bin/bash
# Let's search for distance-related constants and configurations more broadly
rg -i "distance|range" --type java -A 5 -B 5
Length of output: 68422
| @SideOnly(Side.CLIENT) | ||
| public void PlaySoundClient(double distance, SoundEvent soundIn, SoundCategory categoryIn, float volumeIn, float pitchIn, float xIn, float yIn, float zIn) { |
Desoroxxx
left a comment
Desoroxxx
left a comment
There was a problem hiding this comment.
Pretty good for a first PR, this might look like a lot but it ain't a lot is suggestions that will be merged by me and the rest is renames and minor changes.
But there is one big change that I would like to see, it's time for erm actually
☝🏻🤓
So sounds, they travel far, like real far.
I searched a bit on the internet, did a lil math and ball parkin' and I found realistic numbers for
each type of weapons.
(Yes, silencers really work, like really really well)
| Type | Range (Meters) | Silenced Range (Meters) |
|---|---|---|
| HANDGUN | 500 - 1600 | 50 - 100 |
| LONG_GUN | 800 - 1600 | 100 - 200 |
| RIFLE | 800 - 1600 | 100 - 200 |
| SHOTGUN | 800 - 3200 | 100 - 200 |
| CARBINE | 800 - 1600 | 100 - 200 |
| ASSAULT_RIFLE | 800 - 1600 | 100 - 200 |
| BATTLE_RIFLE | 800 - 1600 | 100 - 200 |
| SNIPER_RIFLE | 800 - 1600 | 100 - 200 |
| MACHINE_GUN | 1200 - 2400 | 100 - 200 |
| SUBMACHINE_GUN | 500 - 1200 | 50 - 100 |
Now when playing a sound, the range should be a random number between RANGE MAX - RANGE MIN.
This might get too crazy on server so include a new config which is a multiplier for the range that should be applied on the random number found.
|
|
||
| @Override | ||
| public IMessage onMessage(RealisticSoundClientMessage message, MessageContext ctx) { | ||
| Minecraft.getMinecraft().addScheduledTask(() -> { |
There was a problem hiding this comment.
Use NetworkUtil (See other place in the project it was used with ctrl + shift + f in IDEA and search NetworkUtil)
| @Override | ||
| public IMessage onMessage(RealisticSoundClientMessage message, MessageContext ctx) { | ||
| Minecraft.getMinecraft().addScheduledTask(() -> { | ||
| Minecraft minecraft = Minecraft.getMinecraft(); |
There was a problem hiding this comment.
Use the constant MC with a static import (See other place in the project it was used with ctrl + shift + f in IDEA and search MC)
| ClientSoundPlayer csp = new ClientSoundPlayer(); | ||
| csp.PlaySoundClient(message.getDistance(), message.getSound(), SoundCategory.PLAYERS, message.getVolume(), message.getPitch(), (float)message.getPos().getX(), (float)message.getPos().getY(), (float)message.getPos().getZ()); |
There was a problem hiding this comment.
In this case using a local variable is useless as you aren't using it twice, simply do new ClientSoundPlayer.PlaySoundClient.
Although for better performance get rid of ClientSoundPlayer and simply put the code here since it's the only usage
| import net.minecraftforge.fml.common.network.simpleimpl.IMessageHandler; | ||
| import net.minecraftforge.fml.common.network.simpleimpl.MessageContext; | ||
|
|
||
| public class RealisticSoundClientHandler implements IMessageHandler<RealisticSoundClientMessage, IMessage> { |
There was a problem hiding this comment.
Should be RealisticSoundClientMessageHandler, also should be final
| double distance = Math.sqrt(playercoord.distanceSq(origin)); | ||
| float volume = (float) adjustVolumeForDistance(silencer, distance); | ||
| return new RealisticSound(soundIn, volume, 1.0F); |
There was a problem hiding this comment.
local variables are unecessary here
| public SoundEvent getSound() { | ||
| return sound; | ||
| } | ||
|
|
||
| public float getVolumeIn() { | ||
| return volumeIn; | ||
| } | ||
|
|
||
| public float getPitchIn() { | ||
| return pitchIn; | ||
| } |
…ClientHandler.java
3 participants
📝 Description
adds: 2 handlers | 2 packetMessage | 2 classes
edit: WeaponFireAspect when player shoot, and MWC to add packet, handler
Note: im sorry i didnt make any description on my commit since most of the time i didnt know how the game work
and so i wasnt sure of what i was trying to make, but its pretty easy to understand now because its clear for me:
First: you got the WeaponFireAspect, when a player shoot it send a packet to the server with some details (origin, type of sound, silencer or no, world id), after that, server check who is in the same world to send a packet to client and play the sound with the class ClientSoundPlayer and calculate the volume of the sound using the distance between player and origin of the sound after that we use minecraft existing function to playDelayed sound to the player
Most important commit are from december 6th
🎯 Goals
The aim is for players to hear the sound of weapons from further away and with a delay depending on their distance from the source of the sound.
❌ Non Goals
Using different sound depend on distance, because that isnt realistic
🚦 Testing
Launch: - Server - OfcClient - Client to get 2 clients and 1 server, go at 200 block with gun, shoot with and without silencer
and see the delay and the sound
⏮️ Backwards Compatibility
I coded this on the 1.7 version of the mod so yeah its compatible with new and older version (i guess)
discord link of the video
https://cdn.discordapp.com/attachments/850422558691164201/1314567980193021952/shoottest.mp4?ex=67543e55&is=6752ecd5&hm=9d5c7350414809653e755c344e67468d01835074405f0bdd518a5d16a9c5ce81&
📚 Related Issues & Documents
Nothing to say here
🖼️ Screenshots/Recordings
📖 Added to documentation?
😄 [optional] What gif best describes this PR or how it makes you feel?
After spending almost 50 hours on that shit, i want to kill myself and and the guy who created gradle but everything fine now because it work ! yeay