fix: improve game skill converse #310
Conversation
accept both munged and unmunged intent names intentlayers use the munged name preffixed with skill_id: and was failing to enable/disable intents
accept both munged and unmunged intent names intentlayers use the munged name preffixed with skill_id: and was failing to enable/disable intents
async converse fix pause/playing state check ensure skills are active
WalkthroughThe pull request introduces modifications to two skill-related classes in the OVOS Workshop: Changes
Sequence DiagramsequenceDiagram
participant Skill
participant PlaybackHandler
participant SystemState
Skill->>SystemState: activate()
Skill->>PlaybackHandler: handle_play/resume/pause
PlaybackHandler-->>Skill: Process playback action
Skill->>SystemState: Update playback state
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #310 +/- ##
==========================================
- Coverage 53.41% 49.73% -3.68%
==========================================
Files 37 37
Lines 4362 4355 -7
==========================================
- Hits 2330 2166 -164
- Misses 2032 2189 +157 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ovos_workshop/skills/game_skill.py (3)
127-134: Parameterizing 'calc_intent' with a timeout is a welcome addition.
This new timeout argument provides more flexibility when waiting for the intent service response. As a good-to-have enhancement, you might log or handle scenarios where the wait times out (i.e., no response arrived).
223-228: Asynchronous command handling looks clean.
Defining '_async_cmd' to process game commands separately can improve responsiveness. As a future enhancement, consider adding exception handling to capture errors from 'on_game_command' for more robust debugging/fault tolerance.
257-257: Specific handling of KeyError/IndexError is good practice.
However, consider providing more context in the log message to help with debugging, e.g., which key or index was missing. This can simplify root cause analysis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ovos_workshop/skills/common_play.py(2 hunks)ovos_workshop/skills/game_skill.py(4 hunks)
🔇 Additional comments (5)
ovos_workshop/skills/game_skill.py (3)
Line range hint 195-207: Consistent use of timeout in 'skill_will_trigger'.
Integrating a configurable timeout here ensures uniform behavior with the updated 'calc_intent' method. The default of 0.8 seconds may be optimal for most use cases, but keep in mind potential implications for user responsiveness in slower environments.
229-254: Streamlined 'converse' flow aids reusability and clarity.
• The additional check with 'skill_will_trigger' ensures this method only intercepts utterances that won't be handled by an explicit intent.
• Using 'bus.once(..., _async_cmd)' offloads game command processing and promotes responsiveness.
• The autosave call before handling the command is well-placed. Just be mindful of any blocking I/O that could stall user interactions.
260-260: Robust fallback for unexpected exceptions.
Logging the stack trace is essential. The approach is sound; no further improvements are needed here.
ovos_workshop/skills/common_play.py (2)
395-397: Ensuring skill activation before playback is a solid improvement.
Calling 'self.activate()' before modifying '_playing' or '_paused' states helps maintain a consistent skill lifecycle. This prevents edge cases where playback actions might be invoked on an inactive skill.
421-421: Resuming playback with skill activation.
Similar to the play handler, explicitly activating the skill before resuming ensures a coherent lifecycle. This small addition reduces the risk of resuming a skill that’s not fully ready.
async converse
fix pause/playing state check
ensure skills are active
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
conversemethod for better game state management and utterance processing.on_game_commandfrom theConversationalGameSkillclass.