Reviews #2
Open
Reviews #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
YBoy-git
approved these changes
Apr 11, 2024
vsmysle
reviewed
Apr 12, 2024
There was a problem hiding this comment.
Interesting project! You've done a great job simulating water droplets with the wgpu library 😃
Here are some suggestions for improvement:
- While default configuration values in the code are fine, enhancing your solution to fetch these values from a configuration file (e.g. settings.toml) or environmental variables would be fantastic.
- Consider consolidating both the UI and simulation into a single crate as their functionalities are closely related. However, if you anticipate adding alternative ways to interact with the simulation in the future (e.g., updating configurations from a website), maintaining separate crates might be preferable.
- Hardcoding the TCP port and address might lead to program panics if any other service is bound to port 12345. Providing flexibility in port selection (e.g via config / cli ) could resolve this issue.
- Currently, the entire
settingsis serialized and sent to the simulation even when only one value is updated in the UI. Consider optimizing this process by transmitting only delta updates from the UI to the simulation. - Solution crashes the window manager on macOS. It is worth to debug it and find the root cause of this issue.
- In
main.rs, consider spawning multiple threads for different parts of the simulation instead of spawning commands. This approach could enhance performance and resource utilization.
VictoriaGrasshopper
approved these changes
Apr 14, 2024
nick-maslov
approved these changes
Apr 14, 2024
🤩🤩🤩
|
NamesMark
approved these changes
Apr 14, 2024
Comment on lines
+4
to
+9
| let mut simulation_process = Command::new("target/debug/simulation") | ||
| .env("RUST_LOG", "error") | ||
| .spawn() | ||
| .expect("Failed to spawn simulation"); | ||
|
|
||
| let mut gui_process = Command::new("target/debug/settings_ui") |
There was a problem hiding this comment.
Hi! Is it possible to make it also build/run with --release flag? I see you've hardcoded the debug build. I suspect it may be a bit more performant?
There was a problem hiding this comment.
I doubt it's going to change much, because main logic happens on the GPU. CPU only handles setup.
|
Satisfying
|
divi255
approved these changes
Apr 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.