Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: game skill #306

Merged
merged 17 commits into from
Dec 18, 2024
Merged

feat: game skill #306

merged 17 commits into from
Dec 18, 2024

Conversation

JarbasAl
Copy link
Member

@JarbasAl JarbasAl commented Dec 18, 2024

add new base class to make it easy to implement voice games

example usage:

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced OVOSGameSkill for game playback integration.
    • Added ConversationalGameSkill for handling game commands and user interactions.
    • Implemented methods for game control: play, pause, stop, save, and load.
    • Enhanced search functionality for games within the OCP GUI.
    • Added properties to check if a game is currently active.
    • New user-facing messages for loading and saving games, including notifications for game pause, unpausing, and resuming.
  • Bug Fixes

    • Improved handling of game state and playback events.
    • Added error messages for game loading and saving issues.
  • Documentation

    • Updated documentation to reflect new game skill capabilities and usage.

Copy link
Contributor

coderabbitai bot commented Dec 18, 2024

Walkthrough

The pull request introduces significant updates to the game_skill.py file in the ovos_workshop/skills directory, defining the OVOSGameSkill and ConversationalGameSkill classes. The OVOSGameSkill class integrates with the OpenVoiceOS Common Playback (OCP) framework, implementing methods for game playback control, intent matching, and state management. The ConversationalGameSkill class extends this functionality, providing methods for handling user interactions and game commands. Additionally, several dialog files have been updated to enhance user notifications related to game actions.

Changes

File Change Summary
ovos_workshop/skills/game_skill.py Added OVOSGameSkill class with OCP integration methods, game playback handlers, and state management. Added ConversationalGameSkill class for user interaction handling.
ovos_workshop/locale/en/cant_load_game.dialog Added dialog message: "the game can't be loaded".
ovos_workshop/locale/en/cant_save_game.dialog Added dialog message: "the game can't be saved".
ovos_workshop/locale/en/game_pause.dialog Added dialog message: "game paused".
ovos_workshop/locale/en/game_unpause.dialog Added dialog messages: "unpausing game" and "resuming game".
ovos_workshop/skills/common_play.py Added _paused state management in OVOSCommonPlaybackSkill and updated playback methods.
ovos_workshop/skills/ovos.py Renamed method __handle_stop to _handle_stop in OVOSSkill for event registration consistency.

Possibly related PRs

  • feat:skilljson and homescreen #283: Enhancements to skill metadata integration with the homescreen may relate to OVOSGameSkill for user interaction registration.
  • feat:utterance_modifiers #291: Modification to the speak_dialog method in OVOSSkill could be relevant for customizing how game commands are spoken or processed in OVOSGameSkill.

Suggested reviewers

  • NeonDaniel
  • mikejgray

Poem

🎮 A Rabbit's Game Skill Tale 🐰
In OpenVoiceOS, where skills take flight,
A game framework emerges, shining bright.
Conversational, playful, ready to start,
With OCP magic and a developer's art.
Game on, little voices, let the fun begin! 🕹️


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26b01a2 and 426f851.

📒 Files selected for processing (7)
  • ovos_workshop/locale/en/cant_load_game.dialog (1 hunks)
  • ovos_workshop/locale/en/cant_save_game.dialog (1 hunks)
  • ovos_workshop/locale/en/game_pause.dialog (1 hunks)
  • ovos_workshop/locale/en/game_unpause.dialog (1 hunks)
  • ovos_workshop/skills/common_play.py (5 hunks)
  • ovos_workshop/skills/game_skill.py (1 hunks)
  • ovos_workshop/skills/ovos.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • ovos_workshop/locale/en/cant_save_game.dialog
  • ovos_workshop/locale/en/game_unpause.dialog
  • ovos_workshop/locale/en/game_pause.dialog
  • ovos_workshop/locale/en/cant_load_game.dialog
  • ovos_workshop/skills/ovos.py
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_workshop/skills/game_skill.py

46-46: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

🔇 Additional comments (9)
ovos_workshop/skills/game_skill.py (7)

48-69: LGTM! Well-structured OCP integration.

The _ocp_featured method is well-implemented:

  • Proper type hints and return types
  • Clear documentation
  • Efficient playlist construction
  • Appropriate confidence scoring

71-84: LGTM! Robust search implementation.

The _ocp_search method has:

  • Proper media type filtering
  • Good confidence threshold (50%)
  • Efficient matching using Damerau-Levenshtein similarity

86-92: LGTM! Clean state management.

The state properties are well-implemented:

  • Clear separation of concerns between playing and paused states
  • Thread-safe using Event objects
  • Good type hints

152-169: LGTM! Well-implemented pause/resume handling.

The pause/resume methods:

  • Properly manage state transitions
  • Include user feedback through dialogs
  • Allow customization through settings

246-262: LGTM! Robust deactivation handling.

The deactivation handler:

  • Properly handles paused state
  • Implements auto-save
  • Has good error handling
  • Includes informative logging

33-47: ⚠️ Potential issue

Fix constructor argument ordering.

The constructor violates Python's argument ordering convention. Star args should come before keyword arguments.

Apply this fix:

-    def __init__(self, skill_voc_filename: str,
-                 *args,
-                 skill_icon: str = "",
-                 game_image: str = "",
-                 **kwargs):
+    def __init__(self, skill_voc_filename: str,
+                 skill_icon: str = "",
+                 game_image: str = "",
+                 *args,
+                 **kwargs):

Likely invalid or redundant comment.

🧰 Tools
🪛 Ruff (0.8.2)

46-46: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)


142-151: 🛠️ Refactor suggestion

Add error handling for dialog file loading.

The save/load methods should handle potential errors when loading dialog files.

Apply this fix:

    def on_save_game(self):
        """skills can override method to implement functioonality"""
+       try:
            speech = _get_dialog("cant_save_game", self.lang)
            self.speak(speech)
+       except Exception as e:
+           self.log.error(f"Failed to load save game dialog: {e}")
+           self.speak("Unable to save game")

    def on_load_game(self):
        """skills can override method to implement functioonality"""
+       try:
            speech = _get_dialog("cant_load_game", self.lang)
            self.speak(speech)
+       except Exception as e:
+           self.log.error(f"Failed to load load game dialog: {e}")
+           self.speak("Unable to load game")

Likely invalid or redundant comment.

ovos_workshop/skills/common_play.py (2)

396-402: LGTM! Robust handler parameter validation.

The handler invocation:

  • Properly checks for message parameter support
  • Uses signature inspection for safety
  • Maintains clean state transitions

566-570: LGTM! Clean state management in stop handler.

The stop handler:

  • Properly clears both playing and paused states
  • Maintains parent class behavior
  • Ensures clean state transitions

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added feature and removed feature labels Dec 18, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
ovos_workshop/skills/game_skill.py (3)

39-39: Refactor star-arg unpacking after keyword arguments per style guidelines
Static analysis flags this line due to star-arg usage after keyword arguments. While it may still work, it's generally discouraged for readability and maintainability.

Consider rearranging the arguments or removing the star-arg if not strictly necessary.

-        super().__init__(skill_icon=skill_icon, skill_voc_filename=skill_voc_filename,
-                         supported_media=[MediaType.GAME], *args, **kwargs)
+        super().__init__(
+            skill_voc_filename=skill_voc_filename,
+            skill_icon=skill_icon,
+            supported_media=[MediaType.GAME],
+            *args, **kwargs
+        )
🧰 Tools
🪛 Ruff (0.8.2)

39-39: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)


168-180: Clarify fallback behavior in "will_trigger"
"will_trigger" returns True only if this skill is the top intent. If multiple game skills could exist, a fallback mechanism might be needed to handle partial matches or to disambiguate among games.


197-199: Consider separating abandon and stop actions
"on_abandon_game()" is called immediately before "on_stop_game()" here. If the user’s intention is to return to the main menu (not necessarily to stop the session), you might consider letting them request a resume later.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7791941 and 6c5ae69.

📒 Files selected for processing (1)
  • ovos_workshop/skills/game_skill.py (1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_workshop/skills/game_skill.py

39-39: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

🔇 Additional comments (2)
ovos_workshop/skills/game_skill.py (2)

75-82: Verify that 'Playlist' indexing is supported
Here, "_ocp_featured()" returns a 'Playlist' object, but you're accessing "[0]". Confirm that 'Playlist' implements indexing or if you should access 'Playlist.tracks[0]' instead.

✅ Verification successful

Since we found that Playlist inherits from list in ovos_workshop/backwards_compat.py, it automatically supports indexing operations. The class definition class Playlist(list): clearly shows that Playlist is a subclass of Python's built-in list type, which means it inherits all list functionality including indexing operations.

Indexing operation is supported, code is correct
The current implementation accessing _ocp_featured()[0] is valid since Playlist inherits from list and therefore supports standard list indexing operations. No changes needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Checking if 'Playlist' in ovos_utils.ocp or related references implement indexing.
rg -A 10 "class Playlist" 

Length of output: 738


110-115: Ensure concurrency safety when stopping a running game
The 'stop()' method checks 'is_playing' and invokes 'on_stop_game()' if true. Verify that no concurrency issues arise if multiple stop requests arrive concurrently.

✅ Verification successful

Let me search for more context about the game skill implementation and any threading/concurrency related code.


Let me check the GameSkill class implementation to understand how the _playing Event is used.


Let me try a different approach to find the GameSkill implementation.


Let me check the full game_skill.py file to understand the complete implementation.


Concurrency is properly handled in the game skill implementation

The implementation is thread-safe because:

  1. The is_playing state is managed using a threading Event (self._playing) inherited from OVOSCommonPlaybackSkill
  2. The Event object is thread-safe by design and atomic in its operations
  3. The stop() method's check-then-act pattern is safe because Event's is_set() and the subsequent on_stop_game() call cannot be interrupted
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for concurrency control (threading lock or event usage) in OVOSCommonPlaybackSkill
rg -A 10 "class OVOSCommonPlaybackSkill"

Length of output: 747


Script:

#!/bin/bash
# Search for the game skill class definition and implementation
ast-grep --pattern 'class GameSkill {
  $$$
}'

# Search for any threading/lock related imports and usage
rg -l "import threading|from threading"
rg "Lock\(\)|Event\(\)|RLock\(\)"

# Search for is_playing property/variable definition
rg -A 5 "is_playing"

Length of output: 2749


Script:

#!/bin/bash
# Search for GameSkill class implementation
rg -A 20 "class GameSkill"

# Search for on_stop_game implementation
ast-grep --pattern 'def on_stop_game($$$) {
  $$$
}'

Length of output: 83


Script:

#!/bin/bash
# Search for GameSkill class with different quotes
rg -A 20 'class GameSkill\('

# Search for the file containing GameSkill
fd GameSkill

# Try to find on_stop_game implementation with different pattern
rg -A 5 "def on_stop_game"

Length of output: 857


Script:

#!/bin/bash
# Get the full content of game_skill.py
cat ovos_workshop/skills/game_skill.py

Length of output: 7718

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 4.32099% with 155 lines in your changes missing coverage. Please review.

Project coverage is 49.89%. Comparing base (7c02206) to head (426f851).
Report is 176 commits behind head on dev.

Files with missing lines Patch % Lines
ovos_workshop/skills/game_skill.py 0.00% 127 Missing ⚠️
ovos_workshop/skills/common_play.py 15.15% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #306      +/-   ##
==========================================
- Coverage   53.41%   49.89%   -3.52%     
==========================================
  Files          37       37              
  Lines        4362     4341      -21     
==========================================
- Hits         2330     2166     -164     
- Misses       2032     2175     +143     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JarbasAl added a commit to TigreGotico/ovos-skill-moon-game that referenced this pull request Dec 18, 2024
@github-actions github-actions bot added feature and removed feature labels Dec 18, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (4)
ovos_workshop/skills/game_skill.py (4)

26-27: Track TODOs for OCP pipeline integration

The comments indicate missing intents in the OCP pipeline for game-specific actions (save/load).

Would you like me to create GitHub issues to track the implementation of these game-specific intents in the OCP pipeline?


87-113: Enhance abstract method documentation

The abstract methods would benefit from more detailed docstrings including:

  • Parameters and return types
  • Example usage
  • Error conditions

Would you like me to provide enhanced docstring templates for these methods?


124-127: Consider making the intent timeout configurable

The hardcoded 0.5 second timeout in calc_intent might be too short for some systems or too long for others.

Consider making this configurable:

+    def __init__(self, skill_voc_filename: str,
+                 *args,
+                 skill_icon: str = "",
+                 game_image: str = "",
+                 intent_timeout: float = 0.5,
+                 **kwargs):
+        self.intent_timeout = intent_timeout
+        ...

     def calc_intent(self, utterance: str, lang: str) -> Optional[Dict[str, str]]:
         response = self.bus.wait_for_response(Message("intent.service.intent.get",
                                                      {"utterance": utterance, "lang": lang}),
                                              "intent.service.intent.reply",
-                                             timeout=0.5)
+                                             timeout=self.intent_timeout)

13-207: Consider enhancing the game framework with additional features

The current implementation provides a solid foundation, but consider these enhancements:

  1. Add support for game state persistence
  2. Include game metrics collection (playtime, completion rate, etc.)
  3. Add support for multiplayer games
  4. Consider implementing a game event system for achievements/milestones

Would you like me to provide detailed implementation suggestions for any of these features?

🧰 Tools
🪛 Ruff (0.8.2)

39-39: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6c5ae69 and e098380.

📒 Files selected for processing (1)
  • ovos_workshop/skills/game_skill.py (1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_workshop/skills/game_skill.py

39-39: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

ovos_workshop/skills/game_skill.py Outdated Show resolved Hide resolved
ovos_workshop/skills/game_skill.py Outdated Show resolved Hide resolved
ovos_workshop/skills/game_skill.py Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
ovos_workshop/skills/game_skill.py (2)

177-191: Improve documentation for will_trigger method

The method's docstring could be clearer about its return value and usage pattern.

Consider updating the docstring:

    def will_trigger(self, utterance: str, lang: str) -> bool:
-        """helper to check if this skill would be selected by ovos-core with the given utterance
-
-        useful in converse method
-            eg. return not self.will_trigger
-
-        this example allows the utterance to be consumed via converse of using ovos-core intent parser
-        ensuring it is always handled by the game skill regardless
-        """
+        """Check if this skill would be selected by ovos-core for the given utterance.
+
+        Args:
+            utterance (str): The user's spoken input
+            lang (str): The language code
+
+        Returns:
+            bool: True if ovos-core would select this skill for the utterance,
+                  False otherwise
+
+        Usage:
+            In converse method, use `not self.will_trigger(utterance, lang)`
+            to ensure game commands are handled even when they don't match
+            any specific intent.
+        """

13-133: Consider adding game state management capabilities

The base class provides a solid foundation for game implementation, but could benefit from some additional architectural considerations:

  1. Consider adding a game state management system to help implementations track and persist game states consistently
  2. Think about adding hooks for analytics/telemetry to track game usage and user engagement
  3. Consider adding rate limiting or cooldown mechanisms for game commands to prevent spam

These additions would make the framework more robust for production use.

Would you like assistance in implementing any of these suggestions?

🧰 Tools
🪛 Ruff (0.8.2)

40-40: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e098380 and 0d61ec6.

📒 Files selected for processing (1)
  • ovos_workshop/skills/game_skill.py (1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_workshop/skills/game_skill.py

40-40: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

🔇 Additional comments (5)
ovos_workshop/skills/game_skill.py (5)

1-11: LGTM! Well-organized imports with proper type hints

The imports are comprehensive and properly organized, including all necessary components for OCP integration and type hinting.


47-83: LGTM! Well-implemented OCP integration

The OCP integration is robust with:

  • Proper featured media handling with confidence scoring
  • Efficient game name matching using Damerau-Levenshtein similarity
  • Appropriate media type filtering

192-200: Add error handling in converse method

The converse method should handle potential exceptions when processing utterances.


202-209: Add error handling in handle_deactivate method

The deactivation handler should gracefully handle errors to ensure proper cleanup.


32-40: ⚠️ Potential issue

Fix constructor argument ordering

The constructor violates Python's argument ordering convention by having *args after a keyword argument.

Apply this fix:

    def __init__(self, skill_voc_filename: str,
+                *args,
                 skill_icon: str = "",
                 game_image: str = "",
-                *args, **kwargs):
+                **kwargs):
        self.game_image = game_image
        super().__init__(skill_icon=skill_icon, skill_voc_filename=skill_voc_filename,
                        supported_media=[MediaType.GAME], *args, **kwargs)

Likely invalid or redundant comment.

🧰 Tools
🪛 Ruff (0.8.2)

40-40: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

@github-actions github-actions bot added feature and removed feature labels Dec 18, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
ovos_workshop/skills/game_skill.py (2)

32-47: Consider adding parameter validation in constructor.

The constructor properly initializes the game skill with required parameters, but could benefit from additional validation.

Consider adding validation:

def __init__(self, skill_voc_filename: str,
             *args,
             skill_icon: str = "",
             game_image: str = "",
             **kwargs):
+    if not skill_voc_filename:
+        raise ValueError("skill_voc_filename is required for OCP integration")
    self.game_image = game_image
    super().__init__(skill_icon=skill_icon, skill_voc_filename=skill_voc_filename,
                     supported_media=[MediaType.GAME], *args, **kwargs)
🧰 Tools
🪛 Ruff (0.8.2)

41-41: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)


150-161: Consider adding sound effects for pause/resume.

The pause/resume handlers could benefit from audio feedback.

Consider adding sound effects:

def on_pause_game(self):
    """called by ocp_pipeline on 'pause' if game is being played"""
    # TODO - default dialog/sound
    self._paused.set()
+   self.play_audio('snd/game_pause.mp3', instant=True)
    self.acknowledge()

def on_resume_game(self):
    """called by ocp_pipeline on 'resume/unpause' if game is being played and paused"""
    # TODO - default dialog/sound
    self._paused.clear()
+   self.play_audio('snd/game_resume.mp3', instant=True)
    self.acknowledge()
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d61ec6 and f43b977.

📒 Files selected for processing (5)
  • ovos_workshop/locale/en/cant_load_game.dialog (1 hunks)
  • ovos_workshop/locale/en/cant_save_game.dialog (1 hunks)
  • ovos_workshop/skills/common_play.py (5 hunks)
  • ovos_workshop/skills/game_skill.py (1 hunks)
  • ovos_workshop/skills/ovos.py (2 hunks)
✅ Files skipped from review due to trivial changes (2)
  • ovos_workshop/locale/en/cant_load_game.dialog
  • ovos_workshop/locale/en/cant_save_game.dialog
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_workshop/skills/game_skill.py

41-41: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

🔇 Additional comments (10)
ovos_workshop/skills/game_skill.py (6)

13-30: LGTM! Well-documented class purpose and bus events.

The class documentation clearly explains its purpose, integration with OCP, and the required parameters. The bus events are well documented.


48-69: LGTM! Well-implemented OCP featured media integration.

The _ocp_featured method correctly implements the OCP GUI menu integration with proper media entry configuration.


71-84: LGTM! Robust search implementation with confidence threshold.

The _ocp_search method properly implements search functionality with:

  • Media type validation
  • Confidence scoring
  • Threshold-based filtering

94-120: LGTM! Well-defined abstract methods for game lifecycle.

The abstract methods clearly define the required implementations for game lifecycle management:

  • play_game
  • pause_game
  • resume_game
  • stop_game
  • save_game
  • load_game

140-149: LGTM! Well-implemented default save/load game handlers.

The default implementations properly handle the case when save/load functionality is not implemented by using dialog files.


201-220: LGTM! Robust error handling in converse method.

The converse method implementation includes proper error handling for:

  • KeyError and IndexError for message data access
  • General exception handling with logging
ovos_workshop/skills/common_play.py (3)

76-76: LGTM! Added paused state tracking.

The addition of self._paused = Event() properly tracks the paused state of playback.


395-395: LGTM! Consistent paused state management.

The handlers properly manage the paused state:

  • Cleared on play
  • Set on pause
  • Cleared on resume
  • Cleared on stop

Also applies to: 401-401, 411-411, 437-437


551-555: LGTM! Proper cleanup in stop handler.

The _handle_stop method properly cleans up both playing and paused states.

ovos_workshop/skills/ovos.py (1)

1079-1079: LGTM! Improved method visibility.

The change from __handle_stop to _handle_stop improves method visibility while maintaining encapsulation.

Also applies to: 1267-1267

@github-actions github-actions bot added feature and removed feature labels Dec 18, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
ovos_workshop/skills/game_skill.py (3)

14-31: Consider enhancing class documentation.

While the docstring provides good information about OCP integration, it would be helpful to:

  1. Document the purpose and use cases of the class
  2. Explain the relationship with OVOSCommonPlaybackSkill
  3. Add examples of implementing game skills

33-48: Consider adding validation for game_image path.

The initialization accepts a game_image parameter but doesn't validate if the image file exists. This could lead to runtime errors when displaying the game in the GUI.

def __init__(self, skill_voc_filename: str,
             *args,
             skill_icon: str = "",
             game_image: str = "",
             **kwargs):
    """IMPORTANT: contents of skill_voc_filename are crucial for intent matching
    without that ocp_pipeline might not recognize the skill as a game"""
+   if game_image and not os.path.isfile(game_image):
+       LOG.warning(f"Game image not found: {game_image}")
    self.game_image = game_image
    super().__init__(skill_icon=skill_icon, skill_voc_filename=skill_voc_filename,
                     supported_media=[MediaType.GAME], *args, **kwargs)
🧰 Tools
🪛 Ruff (0.8.2)

42-42: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)


180-195: Consider adding documentation for game command handling.

The on_game_command method would benefit from more detailed documentation about:

  1. Expected format of game commands
  2. How to handle different command types
  3. Examples of command processing
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d61ec6 and 2d52f5c.

📒 Files selected for processing (7)
  • ovos_workshop/locale/en/cant_load_game.dialog (1 hunks)
  • ovos_workshop/locale/en/cant_save_game.dialog (1 hunks)
  • ovos_workshop/locale/en/game_pause.dialog (1 hunks)
  • ovos_workshop/locale/en/game_unpause.dialog (1 hunks)
  • ovos_workshop/skills/common_play.py (5 hunks)
  • ovos_workshop/skills/game_skill.py (1 hunks)
  • ovos_workshop/skills/ovos.py (2 hunks)
✅ Files skipped from review due to trivial changes (4)
  • ovos_workshop/locale/en/game_unpause.dialog
  • ovos_workshop/locale/en/cant_save_game.dialog
  • ovos_workshop/locale/en/cant_load_game.dialog
  • ovos_workshop/locale/en/game_pause.dialog
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_workshop/skills/game_skill.py

42-42: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

🔇 Additional comments (10)
ovos_workshop/skills/game_skill.py (6)

1-12: LGTM! Well-organized imports with proper type hints.

The imports are logically grouped and type hints are used appropriately to improve code documentation.


49-70: LGTM! Well-implemented OCP featured media integration.

The _ocp_featured method correctly implements the OCP interface for displaying the game in the GUI menu.


72-85: LGTM! Robust search implementation with confidence threshold.

The _ocp_search method properly handles game name matching with a reasonable confidence threshold of 50%.


87-121: LGTM! Well-defined abstract game lifecycle methods.

The abstract methods clearly define the game lifecycle (play, pause, resume, stop, save, load) with appropriate docstrings.


153-170: LGTM! Well-implemented pause handling with configurable feedback.

The pause implementation properly manages state and provides configurable user feedback.


247-263: LGTM! Robust deactivation handling with auto-save.

The deactivation handler properly manages game state and includes auto-save functionality.

ovos_workshop/skills/common_play.py (3)

76-76: LGTM! Added pause state tracking.

The _paused Event properly tracks pause state for playback control.


395-395: LGTM! Consistent pause state management.

The playback handlers properly manage the pause state across play, pause, resume, and stop operations.

Also applies to: 401-401, 411-411, 437-437


551-555: LGTM! Clean state reset in stop handler.

The stop handler properly clears both playing and paused states.

ovos_workshop/skills/ovos.py (1)

1079-1079: LGTM! Consistent stop handler naming.

The stop handler registration and implementation maintain consistent naming conventions.

Also applies to: 1267-1267

ovos_workshop/skills/game_skill.py Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
ovos_workshop/skills/game_skill.py (1)

219-223: Consider adding error handling to autosave.

The _autosave method should handle potential errors during the save operation.

Consider this implementation:

def _autosave(self):
    """helper to save the game automatically if enabled in settings.json and implemented by skill"""
-   if self.settings.get("auto_save", False) and self.save_is_implemented:
-       self.on_save_game()
+   try:
+       if self.settings.get("auto_save", False) and self.save_is_implemented:
+           self.on_save_game()
+   except Exception as e:
+       self.log.error(f"Auto-save failed: {e}")
ovos_workshop/skills/common_play.py (1)

394-396: Consider caching handler signatures.

The code repeatedly checks handler signatures in multiple methods. This could be optimized by caching the signatures during initialization.

Consider caching the signatures in the constructor:

def __init__(self, *args, **kwargs):
    # ... existing code ...
+   self.__handler_params = {
+       'playback': signature(self.__playback_handler).parameters if self.__playback_handler else None,
+       'pause': signature(self.__pause_handler).parameters if self.__pause_handler else None,
+       'resume': signature(self.__resume_handler).parameters if self.__resume_handler else None,
+       'next': signature(self.__next_handler).parameters if self.__next_handler else None,
+       'prev': signature(self.__prev_handler).parameters if self.__prev_handler else None
+   }

Then update the handler methods to use the cached signatures:

def __handle_ocp_play(self, message):
    if self.__playback_handler:
-       params = signature(self.__playback_handler).parameters
-       kwargs = {"message": message} if "message" in params else {}
+       kwargs = {"message": message} if self.__handler_params['playback'] and "message" in self.__handler_params['playback'] else {}
        self.__playback_handler(**kwargs)

Also applies to: 408-410, 420-422, 431-433, 440-442

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2d52f5c and 96968e3.

📒 Files selected for processing (2)
  • ovos_workshop/skills/common_play.py (5 hunks)
  • ovos_workshop/skills/game_skill.py (1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_workshop/skills/game_skill.py

46-46: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

🔇 Additional comments (6)
ovos_workshop/skills/game_skill.py (5)

48-69: LGTM: Well-structured OCP integration.

The _ocp_featured method correctly implements the featured media functionality for OCP GUI integration.


71-84: LGTM: Robust search implementation.

The _ocp_search method properly implements game name matching with confidence scoring.


224-244: LGTM: Robust error handling in converse method.

The method properly handles various error cases and includes appropriate logging.


246-262: LGTM: Well-implemented deactivation handling.

The handle_deactivate method includes proper error handling and state management.


33-46: ⚠️ Potential issue

Fix constructor argument ordering.

The constructor violates Python's argument ordering convention by having *args after keyword arguments.

Apply this fix:

def __init__(self, skill_voc_filename: str,
+            *args,
             skill_icon: str = "",
             game_image: str = "",
-            *args, **kwargs):
+            **kwargs):
    self.game_image = game_image
    super().__init__(skill_icon=skill_icon, skill_voc_filename=skill_voc_filename,
                     supported_media=[MediaType.GAME],
                     playback_handler=self.on_play_game,
                     pause_handler=self.on_pause_game,
                     resume_handler=self.on_resume_game,
                     *args, **kwargs)

Likely invalid or redundant comment.

🧰 Tools
🪛 Ruff (0.8.2)

46-46: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

ovos_workshop/skills/common_play.py (1)

564-568: LGTM: Clean stop handling implementation.

The _handle_stop method properly cleans up both playing and paused states.

JarbasAl and others added 7 commits December 18, 2024 15:38
add new base class to make it easy to implement voice games
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
@github-actions github-actions bot added feature and removed feature labels Dec 18, 2024
@JarbasAl JarbasAl merged commit 23ee52a into dev Dec 18, 2024
9 checks passed
JarbasAl added a commit that referenced this pull request Dec 18, 2024
* feat: game skill (#306)

* feat: game skill

add new base class to make it easy to implement voice games

* add resume handler

* resume

* adjust timeout

* Apply suggestions from code review

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* better stop handling

* rename method for clarity

* implement pause/resume

* dont require save/load methods to be implemented

* implement autosave setting

* tweak when autosave happens

* pause game default dialogs«

* fix - cant use self.speak_dialog for bundled resource files from ovos-workshop

* improve non-decorator usage callback handling

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Increment Version to 3.2.0a1

* Update Changelog

---------

Co-authored-by: JarbasAI <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: JarbasAl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant