Skip to content

Make HTN planning budget configurable via cvar#4616

Open
StarlightHost wants to merge 1 commit into
starlight-devfrom
fix/npc
Open

Make HTN planning budget configurable via cvar#4616
StarlightHost wants to merge 1 commit into
starlight-devfrom
fix/npc

Conversation

@StarlightHost

Copy link
Copy Markdown
Contributor

Short description

Make HTN planning budget configurable via cvar

Why we need to add this

Media (Video/Screenshots)

Checks

  • I do not require assistance to complete the PR.
  • Before posting/requesting review of a PR, I have verified that the changes work.
  • I have added screenshots/videos of the changes, or this PR does not change in-game mechanics.
  • I affirm that my changes are licensed under the MIT License and grant permission for use in this repository under its conditions.

Changelog

@StarlightHost StarlightHost requested a review from a team June 1, 2026 00:24
@github-actions github-actions Bot added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. Changes: C# size/S S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. labels Jun 1, 2026
@StarlightHost StarlightHost enabled auto-merge June 1, 2026 00:24
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR converts the NPC HTN planner's wall-clock planning budget from a hardcoded value into a runtime-configurable parameter. A new AdjustableJobQueue class enables mutable timing constraints, a new NPCHTNPlanBudget CVar defines the configuration contract, and HTNSystem is updated to listen for budget changes and propagate them to the job queue.

Changes

HTN Budget Runtime Configuration

Layer / File(s) Summary
CVar contract definition
Content.Shared/CCVar/CCVars.NPC.cs
NPCHTNPlanBudget CVar is added as server-only configuration with default value 0.004f and documentation indicating prior hardcoded behavior.
AdjustableJobQueue wrapper
Content.Server/_Starlight/NPC/HTN/AdjustableJobQueue.cs
Sealed JobQueue subclass stores a mutable _maxTime, overrides the MaxTime property, and exposes SetMaxTime(double) to update the budget at runtime.
HTNSystem CCVar integration
Content.Server/NPC/HTN/HTNSystem.cs
Adds Starlight/CCVar using directives, injects IConfigurationManager, and registers a Subs.CVar listener that calls SetMaxTime whenever NPCHTNPlanBudget changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes


Well done! This is a clean, coherent refactor that elegantly moves configuration out of hardcoded values into a live-adjustable CVar system. The separation of concerns—contract definition, wrapper class, and integration—is well structured and easy to follow. Great attention to adding supporting documentation and clear dependency flow.

🚥 Pre-merge checks | ✅ 6 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
No Magic Numbers ⚠️ Warning HTNSystem.cs contains hardcoded magic number 0.004 in _planQueue = new(0.004) instead of using the named constant from CCVars.NPCHTNPlanBudget. Replace hardcoded 0.004 with CCVars.NPCHTNPlanBudget.DefaultValue or extract as a named constant to eliminate the magic number.
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: making the HTN planning budget configurable via a CCVar instead of hardcoded.
Description check ✅ Passed The description is related to the changeset, though minimal. It repeats the title and includes standard PR checklist items confirming the work is complete and verified.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Hardcoded Ecs Parameters ✅ Passed Global system parameter correctly moved from hardcoded to CVar. Per-entity config is in HTNComponent (PlanCooldown, Enabled, RootTask). This is appropriate architecture.
Avoid Service Locator ✅ Passed PR uses proper dependency injection via [Dependency] attribute for IConfigurationManager instead of service locator pattern; no IoCManager or indirect service resolution found.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
Content.Server/NPC/HTN/HTNSystem.cs (1)

30-34: 💤 Low value

Clean, well-scoped CVar wiring — nicely done. The invokeImmediately: true argument means SetMaxTime runs during Initialize, before the first _planQueue.Process() in UpdateNPC, so there's no window where planning runs at the stale placeholder budget. The floatdouble widening into SetMaxTime(double) is also fine.

Optional only: the 0.004 on Line 33 is a placeholder that's immediately overwritten by the subscription and duplicates the 0.004f default in CCVars.NPCHTNPlanBudget. If the CVar default ever changes, this literal will silently drift (harmlessly, but confusingly). Not blocking — feel free to leave it given the explanatory comment.

Also applies to: 51-54

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Content.Server/NPC/HTN/HTNSystem.cs` around lines 30 - 34, The
AdjustableJobQueue is constructed with a hardcoded 0.004 placeholder that is
immediately overwritten by the CVar subscription in Initialize (via SetMaxTime
invoked with invokeImmediately:true), which can confuse future readers; update
the constructor call for _planQueue (symbol: AdjustableJobQueue _planQueue) to
use the same default backing value as CCVars.NPCHTNPlanBudget (or otherwise
derive the initial value from that CVar) so the literal isn’t duplicated and the
initial state matches SetMaxTime, and ensure the Initialize subscription still
uses invokeImmediately:true for SetMaxTime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@Content.Server/NPC/HTN/HTNSystem.cs`:
- Around line 30-34: The AdjustableJobQueue is constructed with a hardcoded
0.004 placeholder that is immediately overwritten by the CVar subscription in
Initialize (via SetMaxTime invoked with invokeImmediately:true), which can
confuse future readers; update the constructor call for _planQueue (symbol:
AdjustableJobQueue _planQueue) to use the same default backing value as
CCVars.NPCHTNPlanBudget (or otherwise derive the initial value from that CVar)
so the literal isn’t duplicated and the initial state matches SetMaxTime, and
ensure the Initialize subscription still uses invokeImmediately:true for
SetMaxTime.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 0dfb4b31-882d-4c8b-ba9c-ee2be94ea9e0

📥 Commits

Reviewing files that changed from the base of the PR and between b059280 and 3e73fb9.

📒 Files selected for processing (3)
  • Content.Server/NPC/HTN/HTNSystem.cs
  • Content.Server/_Starlight/NPC/HTN/AdjustableJobQueue.cs
  • Content.Shared/CCVar/CCVars.NPC.cs

Comment on lines +17 to +24
// Starlight start
/// <summary>
/// Per-tick wall-clock budget (seconds) the HTN planner is allowed to spend running plan jobs.
/// Lower it to cap NPC planning cost under load. Was a hardcoded 0.004 (4ms) in HTNSystem.
/// </summary>
public static readonly CVarDef<float> NPCHTNPlanBudget =
CVarDef.Create("npc.htn_plan_budget", 0.004f, CVar.SERVERONLY);
// Starlight end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be in Starlight CCVars

@starlightgithub starlightgithub Bot added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. labels Jun 1, 2026
@github-actions github-actions Bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Jun 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changes: C# S: Awaiting Changes Status: Changes are required before another review can happen S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants