chore: add configurable tick rate from envar or toml config file #830
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a new field, Changes
Sequence Diagram(s)sequenceDiagram
participant Config as WorldConfig
participant World as World
participant TickTimer as Tick Channel
Config->>World: Provide configuration with CardinalTickRate
alt CardinalTickRate > 0
World->>TickTimer: Calculate tick interval (time.Second / CardinalTickRate) and set tickChannel
else Default
World->>TickTimer: Use default tickChannel settings
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🪧 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 (
|
How to use the Graphite Merge QueueAdd the label graphite/merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #830 +/- ##
==========================================
+ Coverage 57.83% 57.94% +0.10%
==========================================
Files 145 145
Lines 9384 9387 +3
==========================================
+ Hits 5427 5439 +12
+ Misses 3506 3501 -5
+ Partials 451 447 -4
|
There was a problem hiding this comment.
This needs to be "CARDINAL_TICK_RATE" following the environment variable convention.
There is also a cardinal option WithTickChannel that can be used here (https://world.dev/cardinal/game/world/api-reference#withtickchannel) instead of modifying a private field directly
|
ok, got it will change to I'm modifying it directly because the WithTickChannel option will be used to override everything, it has the highest priority. |
zulkhair
force-pushed
the
daim/configurable_tick_rate
branch
from
d2a6552 to
0c3c648
Compare
![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 (1)
cardinal/config_test.go (1)
39-39: Consider adding test cases for edge values.The test configuration looks good. Consider adding test cases for edge values (0, max uint64) to ensure proper handling of boundary conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cardinal/config.go(2 hunks)cardinal/config_test.go(2 hunks)cardinal/world.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cardinal/world.go
- cardinal/config.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
cardinal/config_test.go (2)
51-51: LGTM!The environment variable setup is correct and consistent with the naming convention used for other configuration fields.
58-61: Add validation tests for CardinalTickRate.Consider adding test cases to validate the tick rate configuration, similar to how other fields are validated. This could include:
- Test case for valid tick rate (> 0)
- Test case for invalid tick rate (0 or negative)
- Test case for interaction with WithTickChannel option (mentioned in PR objectives)
Here's a suggested test structure:
func TestWorldConfig_Validate_TickRate(t *testing.T) { testCases := []struct { name string cfg WorldConfig wantErr bool }{ { name: "Valid tick rate", cfg: defaultConfigWithOverrides(WorldConfig{CardinalTickRate: 10}), wantErr: false, }, { name: "Zero tick rate", cfg: defaultConfigWithOverrides(WorldConfig{CardinalTickRate: 0}), wantErr: false, // Assuming 0 is valid as per PR objectives }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { err := tc.cfg.Validate() if tc.wantErr { assert.IsError(t, err) } else { assert.NilError(t, err) } }) } }
Merge activity
|
Closes: PLAT-90 ## Overview Based on the user stories, user can configure tick rate from world forge / world cli. Need capabilities to configure tick rate for world engine from outside the code. Added `CARDINAL_TICK_RATE` config that can be put on env variable or using toml config file ## Brief Changelog - Added `CARDINAL_TICK_RATE` var on WorldConfig - Set `world.tickChannel` if `CARDINAL_TICK_RATE > 0` the value can be overridden by `WithTickChannel` option if provided. ## Testing and Verifying Manually tested using starter-game-template. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a configuration option to dynamically adjust the simulation tick rate, enabling users to fine-tune the game loop's performance. - Added support for specifying the tick rate via environment variables for enhanced configurability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
bot
force-pushed
the
daim/configurable_tick_rate
branch
from
0c3c648 to
2d429cc
Compare
|
approved -- i'll caveat a small nit that the ordering of the variable declaration does not spark joy (i intentionally grouped them together so its cleaner) |
Closes: PLAT-90
Overview
Based on the user stories, user can configure tick rate from world forge / world cli.
Need capabilities to configure tick rate for world engine from outside the code.
Added
CARDINAL_TICK_RATEconfig that can be put on env variable or using toml config fileBrief Changelog
CARDINAL_TICK_RATEvar on WorldConfigworld.tickChannelifCARDINAL_TICK_RATE > 0the value can be overridden by
WithTickChanneloption if provided.Testing and Verifying
Manually tested using starter-game-template.
Summary by CodeRabbit