feat: save/load game intent #44
Conversation
improve intent matching when games are the active media
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 24 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 (1)
WalkthroughThis pull request introduces enhancements to the Open Voice OS intent recognition system, focusing on improving natural language processing flexibility. The changes span multiple files in the Changes
Possibly related PRs
Poem
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: 2
🧹 Nitpick comments (3)
ocp_pipeline/opm.py (2)
40-40: Add docstring for skill_id field
It would be helpful to add a short docstring or inline comment explaining the purpose and usage of the new skill_id field so that maintainers and contributors have more context.
869-875: New _update_player_skill_id method
A concise helper method to avoid repeated code. Great for readability. Consider adding a quick docstring clarifying the usage in different states (e.g., if skill_id is missing).ocp_pipeline/locale/en-us/prev.intent (1)
5-6: Consider consolidating similar patternsThe new patterns look good and improve natural language processing flexibility. However, there's some redundancy that could be simplified.
Consider consolidating similar patterns using optional groups:
-previous (song|track|music|movie|video|tune) -play [the] previous (song|track|music|movie|video|tune) +[play [the]] previous (song|track|music|movie|video|tune)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
ocp_pipeline/locale/en-us/load_game.intent(1 hunks)ocp_pipeline/locale/en-us/next.intent(1 hunks)ocp_pipeline/locale/en-us/open.intent(1 hunks)ocp_pipeline/locale/en-us/pause.intent(1 hunks)ocp_pipeline/locale/en-us/prev.intent(1 hunks)ocp_pipeline/locale/en-us/read.intent(1 hunks)ocp_pipeline/locale/en-us/resume.intent(1 hunks)ocp_pipeline/locale/en-us/save_game.intent(1 hunks)ocp_pipeline/opm.py(14 hunks)requirements.txt(1 hunks)translations/en-us/intents.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🔇 Additional comments (25)
ocp_pipeline/opm.py (16)
51-51: Extended intent list
The addition of save_game.intent and load_game.intent looks good. Please ensure they're consistently handled throughout the code (e.g., testing, fallback scenarios, etc.).
191-192: Handle new game intent events
Good job adding the new handlers. Make sure the calling code and surrounding environment are properly updated and tested (e.g. user prompts or CLI commands that might trigger the new events).
295-295: Invocation of _update_player_skill_id
This line ensures that the player's skill context is updated when a new track state is reported, which is beneficial for maintaining skill context integrity.
355-369: Conditional logic for restricting game vs. non-game intents
Nice addition to selectively enable or disable certain playback commands based on whether a game is currently playing.
558-565: New game save/load intent handlers
Both methods correctly forward messages to skill-specific save/load operations. Confirm that skill_id is always set; otherwise, these calls could fail silently if skill_id is None.
605-606: Best match assignment
Assigning best.skill_id to player.skill_id ensures the correct skill context is always tracked during playback. Looks good.
636-636: Skill context reset
Clearing player.skill_id here prevents potential confusion when the user stops media. This is a good practice to avoid stale skill references.
667-667: Maintain skill context upon pause
Properly updating the skill context if needed after pause. This is consistent with your approach in handle_save_intent/load_intent.
680-680: Resume maintains skill context
Same logic as in pause. This approach prevents orphaned references and bug scenarios where skill_id is lost inadvertently.
1098-1098: Skill tracking upon legacy play
Good approach to maintain skill context even for legacy audio.
1108-1108: Clearing skill_id on stop
Same best-practice approach as in handle_stop_intent. Maintains consistent skill references.
1116-1116: Skill context update on legacy pause
Helps maintain consistent references across different states.
1124-1124: Skill context on legacy resume
Ensures the correct skill is re-associated once playback continues.
1132-1132: Legacy audio start
Same pattern for skill context. Consistency across these states is beneficial.
1140-1140: Set skill_id to None when media ends
A final reset. This ensures that we don’t carry a stale skill context and cause confusion in subsequent actions.
317-317:
Potential media type assignment bug
Here, you set player.media_type = MediaType(pstate) instead of using mtype (which is the variable meant to represent media_type). This may cause incorrect media type handling.
- player.media_type = MediaType(pstate)
+ player.media_type = MediaType(mtype)Likely invalid or redundant comment.
ocp_pipeline/locale/en-us/save_game.intent (1)
1-2: New save game phrases
These straightforward additions increase the flexibility of user commands. Ensure your test suites cover both singled-word "save" and the more explicit "save the game" phrasing.
ocp_pipeline/locale/en-us/load_game.intent (1)
1-2: New load game phrases
Similar to save_game.intent, these new phrases give users more command options. Thank you for ensuring consistent usage between saving and loading.
ocp_pipeline/locale/en-us/pause.intent (1)
2-2: Extended pause intent for games
Allowing “pause [the] game” is a logical extension. Make sure to confirm it doesn’t conflict with other game-specific skills or states.
ocp_pipeline/locale/en-us/read.intent (1)
1-1: LGTM! Consider maintaining pattern consistency across all intent files.
The addition of the optional article "[the]" improves natural language processing flexibility. This pattern should be consistently applied across all similar intent files.
✅ Verification successful
Let me check the actual patterns in these files to ensure they are consistently using optional articles.
Optional article "[the]" is consistently used across intent files
The verification shows that the optional article "[the]" is already consistently implemented across all related intent files in the codebase, including media playback, game operations, and reading commands. The pattern follows the same style throughout, making the current change in read.intent align perfectly with the existing conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if similar patterns in other intent files also use optional articles
fd -e intent -x grep -l "\[the\]" {} \; | sort
Length of output: 384
Script:
#!/bin/bash
# Check the actual patterns in intent files that use optional articles
fd -e intent -x grep -H "\[the\]" {} \;
Length of output: 779
ocp_pipeline/locale/en-us/resume.intent (1)
2-2: LGTM! Verify corresponding pause intent for consistency.
The addition of "game" to media types and optional article "[the]" aligns well with the PR's game support objectives.
✅ Verification successful
Verified: pause.intent is consistent with game support
The pause.intent file correctly includes "game" in its pattern, matching the resume.intent changes. The implementation is part of a broader game support feature set, evidenced by related files like load_game.intent, save_game.intent, and GameKeyword.voc in the en-us locale.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if pause.intent has matching patterns
echo "Checking pause.intent for matching game support:"
rg -l "pause.*game" "ocp_pipeline/locale/**/pause.intent"
# Check other related intents for game support
echo "Checking other media control intents for game support:"
rg -l "game" "ocp_pipeline/locale/**/*.(intent|dialog)"
Length of output: 498
Script:
#!/bin/bash
# Let's first check if the directory structure is different
echo "Checking directory structure:"
fd -t d "locale"
echo -e "\nChecking for intent files:"
fd -e intent
echo -e "\nLet's check the content of pause intent if found:"
fd -e intent "pause" --exec cat {}
Length of output: 6525
Script:
#!/bin/bash
# Let's specifically check the content of pause.intent in en-us locale
echo "Content of en-us pause.intent:"
cat ocp_pipeline/locale/en-us/pause.intent
# Also check if there are any other game-related intents
echo -e "\nOther game-related intents:"
rg -l "game" ocp_pipeline/locale/en-us/
Length of output: 495
ocp_pipeline/locale/en-us/open.intent (1)
4-4: LGTM!
The addition of optional article "the" improves natural language processing flexibility while maintaining consistency with other intent patterns.
translations/en-us/intents.json (3)
26-28: Maintain pattern consistency across next/prev intents
The next and prev intent patterns should mirror each other for consistency. Current implementation looks good, but ensure this symmetry is maintained in future updates.
Also applies to: 55-57
36-39: Verify game state handling in pause/resume pipeline
The addition of game support to pause/resume intents is good, but we should verify that the pipeline properly handles game states.
Also applies to: 64-67
✅ Verification successful
Let me try another search strategy to verify the game state handling.
Game state is properly handled through the generic player state system
The verification shows that game states are handled through the generic PlayerState enum in ocp_pipeline/opm.py, which manages all media types including games. The pipeline uses a unified state management system that handles pause/resume operations through:
PlayerStateenum (PLAYING, PAUSED, STOPPED)MediaStateenum for media status tracking- Proper state transitions in pause/resume handlers
The addition of "game" to the intents is consistent with the existing implementation, as the pipeline already handles state management in a media-type agnostic way.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for game state handling in the pipeline
echo "Searching for game state handling..."
rg -l 'game.*state|state.*game' --type python
# Check for pause/resume implementations
echo "Checking pause/resume implementations..."
ast-grep --pattern 'def (pause|resume).*game.*:
$$$
'
Length of output: 310
Script:
#!/bin/bash
# Search for files containing pause/resume functionality
echo "Searching for pause/resume handlers..."
rg -l "pause|resume" --type-add 'code:*.{py,js,ts}'
# Look for game-related state management
echo "Checking for game state management..."
rg -l "game" --type-add 'code:*.{py,js,ts}'
# Search for state management patterns
echo "Looking for state management patterns..."
rg -C 3 "state.*=|setState|getState" --type-add 'code:*.{py,js,ts}'
Length of output: 10806
2-4: Consider media type consistency
The new featured.intent introduces a {media} variable. Ensure this variable's possible values are documented and consistent with other media types used throughout the intents.
✅ Verification successful
Let me gather more information about the media types and their definitions.
Let's check the content of the vocab files to see if they define the media types.
Based on the analysis of the vocabs.json files, I can now provide a final response:
The {media} variable appears to be well-defined through vocabulary files
The {media} variable is consistently used across all language translations for featured intents, and its possible values are clearly defined in the vocab files. The media types include:
- Video content: movies, series, TV shows, documentaries, cartoons, anime
- Audio content: music, audiobooks, podcasts, radio
- Other media: comic books, games
The implementation is consistent with the media types used throughout the intent system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for media type definitions
echo "Searching for media type definitions..."
rg -l 'media.*type|type.*media' --type python
# Check for {media} variable usage
echo "Checking {media} variable usage..."
rg '\{media\}' --type-add 'intent:*.intent'
Length of output: 4566
Script:
#!/bin/bash
# Search for media entity definitions
echo "Searching for media entity definitions..."
rg -l 'media' --type json
# Show content of relevant entity files
echo "Content of entity files:"
fd -e json -x cat {} \; | rg -A 5 -B 5 '"media":'
# Search for vocab files that might define media types
echo "Searching for vocab files..."
fd vocab
Length of output: 1346
Script:
#!/bin/bash
# Check content of vocab files for media-related entries
echo "Content of en-us vocab file:"
cat translations/en-us/vocabs.json
# Search for media-related entities in all vocab files
echo "Searching for media in vocab files:"
rg -A 5 -B 5 '"media"' translations/*/vocabs.json
Length of output: 2816
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
translations/en-us/intents.json (2)
21-22: Maintain consistency in optional articlesThe next intent uses
[the]inconsistently across its phrases. Consider adding the optional article to all relevant phrases for consistency.-"next (song|track|music|movie|video|tune)", +"[the] next (song|track|music|movie|video|tune)", "(play|go to) [the] next (song|track|music|movie|video|tune)"
65-72: Enhance game state management intentsThe save and load game intents could be more comprehensive to better support various gaming scenarios.
Consider:
- Adding support for named save slots
- Including auto-save related commands
- Supporting save state management (delete, list)
- Adding confirmation phrases
Example additions:
"save_game.intent": [ "save", "save [the] game", "save [the] game [as|to] {slot}", "save [the] game [with] {description}", "create [a] [new] save [point|state]", "quick save" ], "load_game.intent": [ "load", "load [the] game", "load [the] (last|previous) [saved] game", "load [the] game [from] {slot}", "load [the] (auto|quick) save", "restore [the] game [to] [the] last save [point]" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ocp_pipeline/locale/en-us/load_game.intent(1 hunks)ocp_pipeline/locale/en-us/next.intent(1 hunks)ocp_pipeline/locale/en-us/prev.intent(1 hunks)translations/en-us/intents.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ocp_pipeline/locale/en-us/prev.intent
- ocp_pipeline/locale/en-us/next.intent
🔇 Additional comments (1)
ocp_pipeline/locale/en-us/load_game.intent (1)
1-3: Consider expanding load game capabilities
The current implementation provides good basic coverage for loading games. However, consider expanding the intent phrases to support more specific loading scenarios.
Consider adding phrases like:
load
load [the] game
load [the] (last|previous) [saved] game
+load [the] game [from] {slot}
+load [the] game [from] {timestamp}
+load [the] save [named|called] {description}
translations/en-us/intents.json
Outdated
| ], | ||
| "resume.intent": [ | ||
| "(unpause|resume)", | ||
| "(unpause|resume|continue|restart) (music|song|track|video|media|playback)", | ||
| "(unpause|resume|continue|restart) (music|song|track|video|media|playback|game)", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align game-related phrases across intents
The resume and pause intents handle games differently. Consider using consistent phrasing for game-related commands.
"(unpause|resume|continue|restart) (music|song|track|video|media|playback|game)"
-"pause [the] (music|song|track|video|media|playback|game)"
+"pause [the] (music|song|track|video|media|playback|game|gameplay)"Also applies to: 44-44
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
translations/en-us/intents.json(2 hunks)
🔇 Additional comments (6)
translations/en-us/intents.json (6)
21-22: LGTM! Well-structured intent patterns
The new patterns maintain consistency while adding flexibility with the optional article "the".
30-30: LGTM! Natural language enhancement
The optional article improves the natural language support while maintaining pattern consistency.
38-38: LGTM! Symmetric with next.intent patterns
The pattern maintains consistency with next.intent and improves natural language support.
44-44: Consider adding "gameplay" as an alternative
While the current pattern is good, consider adding "gameplay" as an alternative for better game-related command coverage.
-"pause [the] (music|song|track|video|media|playback|game)"
+"pause [the] (music|song|track|video|media|playback|game|gameplay)"50-50: LGTM! Comprehensive media control coverage
The pattern provides good coverage for various media-related commands with natural language support.
65-72: Consider enhancing game state management capabilities
While the basic save/load functionality meets the PR objectives, consider expanding the capabilities for a better user experience.
"save_game.intent": [
"save",
- "save [the] game"
+ "save [the] game [as|to] {slot}",
+ "save [the] game [with] {description}"
],
"load_game.intent": [
"load",
- "load [the] game"
+ "load [the] game [from] {slot}",
+ "load [the] (last|previous) [saved] game"
]
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
improve intent matching when games are the active media
NOTE: skills dont need to report if they implemented save functionality or not, a default error dialog is implemented as part of the base class, we should still match the intent if that is what the user wants regardless of being able to fulfill the order or not
OpenVoiceOS/OVOS-workshop#306
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation