Skip to content

Fix dice roll and timer in Test#16

Merged
atomic-junky merged 4 commits into
mainfrom
bugfix/roll-timer
Aug 12, 2024
Merged

Fix dice roll and timer in Test#16
atomic-junky merged 4 commits into
mainfrom
bugfix/roll-timer

Conversation

@RailKill

@RailKill RailKill commented Aug 12, 2024

Copy link
Copy Markdown
Collaborator

Just some minor fixes that I found in Test, taking a break after this.

Bug Fixes

  • Fix DiceRoll never triggering Fail path because random number generator in MonologueProcess.gd was always rolling 100.
  • Fix next() being called by timer expiry, even after the user has skipped the timer by pressing ui_accept. Basically, the timer does not stop counting down even after a dialogue skip, which will make it call next() one extra time.

Features

  • Add an [INFO] notification that displays the timer. This notification lasts as long as the timer, and will close when it is skipped. This doesn't affect other [DEBUG] notification behavior, which still display for its full 7.5 seconds even on skips.

timnot

Summary by Sourcery

Fix issues with DiceRoll and timer behavior in Test. Add an [INFO] notification for timer duration that closes when skipped, and ensure the random number generator in MonologueProcess.gd functions correctly. Prevent next() from being called by timer expiry after a dialogue skip.

New Features:

  • Add an [INFO] notification that displays the timer duration and closes when skipped, without affecting other [DEBUG] notifications.

Bug Fixes:

  • Fix DiceRoll never triggering the Fail path by ensuring the random number generator in MonologueProcess.gd does not always roll 100.
  • Fix next() being called by timer expiry even after the user has skipped the timer by pressing ui_accept, preventing an extra call to next().

@sourcery-ai

sourcery-ai Bot commented Aug 12, 2024

Copy link
Copy Markdown

Reviewer's Guide by Sourcery

This pull request addresses two main issues: fixing the dice roll logic to ensure the fail path can be triggered, and correcting the timer behavior to stop counting down when skipped. Additionally, it introduces a new feature to display INFO notifications that last as long as the timer. The changes involve refactoring notification handling, adding timer management, and ensuring the RNG is properly randomized.

File-Level Changes

Files Changes
Test/Scripts/Notification.gd
Test/Scripts/Main.gd
Test/Scripts/MonologueProcess.gd
Refactored notification handling to support INFO notifications and fixed timer behavior to stop on skip. Improved RNG for dice rolls and added timer management in monologue process.

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 sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Overall Comments:

  • Consider adding additional thread safety measures for the timer modifications to prevent potential race conditions.
  • The _process_node function in MonologueProcess.gd handles many different node types. Consider refactoring this into smaller, more focused functions for better maintainability and potential performance improvements.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 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 ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

notify(text, "INFO", Color.DODGER_BLUE, time)


func notify(text, tag, color, time):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: Consider using a more type-safe approach for color parameters

Instead of accepting any value for color, you could use a Color type hint or create an enum for predefined colors. This would prevent potential runtime errors from invalid color values.

Suggested change
func notify(text, tag, color, time):
func notify(text: String, tag: String, color: Color, time: float) -> void:

@RailKill RailKill requested a review from atomic-junky August 12, 2024 01:20

@atomic-junky atomic-junky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks!

@atomic-junky atomic-junky merged commit abb7d30 into main Aug 12, 2024
@RailKill RailKill deleted the bugfix/roll-timer branch August 17, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants