Refactor and improve MonologueProcess#27
Merged
Conversation
Reviewer's Guide by SourceryThis pull request refactors and improves the MonologueProcess class, enhancing its functionality and preparing it for future adaptations to other engines and languages. The changes focus on improving code organization, expanding conditional text processing capabilities, and adding support for additional audio formats. The PR also includes extensive unit tests and updates to build configurations and CI paths. File-Level Changes
Tips
|
There was a problem hiding this comment.
Hey @RailKill - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
There was a problem hiding this comment.
Hey @RailKill - I've reviewed your changes and they look great!
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
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Collaborator
|
I'll test that as soon as I can but it looks awesome 👍 |
atomic-junky
approved these changes
Aug 26, 2024
atomic-junky
left a comment
Collaborator
There was a problem hiding this comment.
Sorry for the late review!
This file contains hidden or 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
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.
This PR cleans up and separates functions a little more clearly in MonologueProcess in preparation for future adaptations to other engines/languages as part of discussion #24. Having well-defined, atomic operations in its own function help with future extensibility, adaptability and testability, makes it more API-like.
Features
Improved conditional text processing in NodeSentence.
Previous Behavior
You can have
{{ if my_variable then something else another }}in NodeSentence, and ifmy_variableis true,somethingwill be emitted inmonologue_sentence(). Only boolean variables were supported.New Behavior
You can now nest and evaluate expressions in the conditional text. See
Test/Unit/TestMonologueProcess.gdfor automated test examples. Something like:will work. Any words in curly braces will be substituted with variable values if they have the same name unless they are surrounded by quotation marks which indicate they are Strings to be parsed as-is. If they do not exist as variables, you don't need quotation marks.
Support runtime loading of .wav and .ogg audio files in
SfxLoader. Previously, only .mp3 is supported.Notification.gd'sprint()now excludes color codes and BB tags, making console logs cleaner and easier to read.Bug Fixes
Developer Notes
SfxPlayer.gdhas been removed to streamline things.SfxLoaderandAudioStreamPlayer2Dis sufficient.kalimba.wavandmystery_sting.oggfor unit test were recorded by me. I played the instruments, no copyright issues.Summary by Sourcery
Refactor MonologueProcess for better clarity and future extensibility, enhance conditional text processing, and add support for additional audio formats. Improve console log readability and streamline audio handling by removing SfxPlayer.gd. Update build configurations and CI workflow, and introduce extensive unit tests for MonologueProcess.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests: