New Voiceline in NodeSentence

[atomic-junky]

Summary by Sourcery

Add a new voiceline feature to NodeSentence, allowing nodes to specify and handle voiceline paths. Introduce new components for file path selection and validation, and implement a global file dialog system. Refactor the notification system for consistent logging and enhance the SfxLoader for better error handling. Improve path management by adding functions to convert between absolute and relative paths.

New Features:

• Introduce a new voiceline feature in the NodeSentence, allowing the specification and handling of voiceline paths for nodes.
• Add a FilePickerLineEdit component to facilitate file path selection and validation within the UI.
• Implement a GlobalFileDialog class to handle global file dialog operations for saving and opening files.

Enhancements:

• Refactor the notification system to use a unified _notify function with different notification levels, improving consistency in logging messages.
• Enhance the SfxLoader to return error codes when file loading fails, improving error handling.
• Add functionality to convert absolute file paths to relative paths and vice versa, enhancing path management.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request introduces support for voiceline paths in sentence nodes and enhances error handling and type safety in node processing. The changes are implemented through the following key modifications:

1. A new FilePickerLineEdit class is introduced to handle file selection with validation and relative path conversion.
2. The SentenceNodePanel, NodeSentence, and MonologueProcess classes are updated to support voiceline paths.
3. Error handling is improved by introducing a new NotificationLevel enum and updating the notification system.
4. The SfxLoader and SfxPlayer classes are refactored for better type safety and error handling.
5. The ChoiceNode class is updated to filter non-GraphNode children.
6. New utility classes and functions are added for file path handling and global file dialog management.

These changes aim to improve the robustness and functionality of the dialogue system, particularly in handling audio files and providing better user feedback.

File-Level Changes

Files	Changes
Test/Scripts/MonologueProcess.gd Test/Scripts/Main.gd Test/Scripts/Notification.gd	Introduced a NotificationLevel enum and updated the notification system for better error handling
Scripts/SidePanelNodes/SentenceNodePanel.gd Scripts/GraphNodes/NodeSentence.gd	Added support for voiceline paths in sentence nodes
Test/Scripts/SfxLoader.gd Test/Scripts/SfxPlayer.gd	Refactored SfxLoader and SfxPlayer for improved type safety and error handling
Scripts/SubComponents/FilePickerLineEdit.gd	Implemented a new FilePickerLineEdit class for file selection with validation
Scripts/Global/Path.gd	Added utility functions for file path handling
Scripts/GlobalFileDialog.gd Scripts/FilePickerButton.gd	Implemented a global file dialog management system
Scripts/GraphNodes/ChoiceNode.gd	Updated ChoiceNode to filter non-GraphNode children

---

Tips

• Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
• Continue your discussion with Sourcery by replying directly to review comments.
• You can change your review settings at any time by accessing your dashboard:
  • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
  • Change the review language;
• You can always contact us if you have any questions or feedback.

[sourcery-ai]

Hey @atomic-junky - I've reviewed your changes - here's some feedback:

Overall Comments:

• Consider adding more robust error handling and user feedback, particularly in the FilePickerLineEdit class and when dealing with file operations.
• Improve type safety by adding more explicit type annotations, especially for function parameters and return values.
• Add more comprehensive documentation for new functions and classes, including detailed comments for complex algorithms like absolute_to_relative in Path.gd.

Here's what I looked at during the review

• 🟡 General issues: 3 issues found
• 🟡 Security: 2 issues found
• 🟢 Testing: all looks good
• 🟢 Complexity: all looks good
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

[atomic-junky]

So, I've written a function that converts an absolute path to a relative path, normally it works but I'd like to be sure.

Also, in Godot 4.3 there's a MusicPlayer I think (not an AudioPlayer), and I think it would be a good idea to separate Sfx and Music, but when I tried to do it with the 1st Release Candidate, there were some weird bugs, so I think I'll have to wait for an official release.

Otherwise, I've tried to make the code and nodes easily reusable for implementation in other nodes.

I also think there are some big conflicts with the main branch, so I'll have to see how to adapt the code.

[RailKill]

Let me know if you need help merging main into this branch.

[atomic-junky]

I think I've made all the changes you mentioned. I redid the notification system but I'm not sure it's very clean/practical but it probably deserves its own branch.
On the other hand, how do you resolve conflicts, do you try to merge main into this pr or the opposite? (I'm a bit of a noob with git)

[RailKill]

Looks good! Now you just have to merge main into your feature/audio-player branch. Seems like the conflicts are too complex for GitHub, so you have to do it locally. You can do this by:

1. git checkout main and git pull to make sure your local main branch it is up to date.
2. git checkout feature/audio-player to switch to this branch.
3. git merge main to merge main into this branch.
4. At this point, merge is temporarily suspended due to conflicts. Files which have conflicts will have the following sections:

   <<<<<<< HEAD:[[FILENAME]]
   
   [[STUFF IN YOUR CURRENT BRANCH]]
   
   =======
   
   [[STUFF IN MAIN]]
   
   >>>>>>> main:[[FILENAME]]
   

5. You have to see which parts of your code in [[STUFF IN YOUR CURRENT BRANCH]] you want to move into [[STUFF IN MAIN]]. You may have to modify them in a way that combines them both, or delete some parts that are no longer relevant. Be careful not to break existing features.
6. The conflict is resolved when the sections are no longer there, meaning you have removed <<<<<<< HEAD:[[FILENAME]], ======= and >>>>>>> main:[[FILENAME]] and the code looks proper.
7. Once all conflicts are resolved, git add -A to stage all files for the merge commit.
8. git commit to resume the suspended merge commit.
9. Be sure to test the application and ensure that no features were broken as a result of this merge, and that it runs fine without crashes. If there are problems, just fix and commit until it's working as expected.
10. git push to push changes to GitHub.
11. Check back this GitHub PR and it should say "ready to merge"
12. Merge the PR into main.

[RailKill]

Extra notes: You have to merge main into this branch first because this branch is outdated, i.e. the history is messed up. So we're taking the new UndoRedo stuff from main into here to fix that. Once we're up to date, main still doesn't have the FilePicker stuff from this branch yet, so the PR is to merge this branch back to main, then everything will be in sync.

Merging conflicts are a common source for bugs. That's why there is automated testing in software development. If in future we start to encounter regression bugs (bugs that reoccur in newer versions even after they were previously fixed), we might wanna consider a unit test framework like gdUnit4 to prevent PRs that break old features. Happy merging!

[atomic-junky]

Normally everything is good, but I'd like you to test it to be sure

[sourcery-ai]

Hey @atomic-junky - I've reviewed your changes - here's some feedback:

Overall Comments:

• Consider breaking this PR into smaller, more focused changes. The current scope, touching multiple files with both functional and organizational changes, makes it challenging to review effectively.
• The path handling functions in Path.gd look useful, but ensure thorough testing across different operating systems and file systems to catch potential edge cases.

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟢 Complexity: all looks good
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}