Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Ensures the room combiner’s current scenario is set when manually applying a combination scenario, preventing CurrentScenario/_currentScenario from remaining null in manual mode flows.
Changes:
- After manually applying partition states for a scenario, calls
ChangeScenario(scenario)to set/activate the scenario. - Minor formatting-only change within the partition loop block.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| // Activates the scenario to prevent _currentScenario from being null when starting in manual mode. | ||
| ChangeScenario(scenario); |
There was a problem hiding this comment.
ChangeScenario returns Task (async) and is being invoked without awaiting or otherwise observing the task. This can hide exceptions from Activate/Deactivate and can also allow overlapping scenario changes if this method is called multiple times quickly. Consider explicitly firing-and-forgetting with exception logging (e.g., capturing the returned task and handling faults) or introducing a serialized/queued scenario-change mechanism so only one activation runs at a time.
Suggested change
| ChangeScenario(scenario); | |
| var _ = ChangeScenario(scenario).ContinueWith(t => | |
| { | |
| var ex = t.Exception != null ? t.Exception.Flatten() : null; | |
| if (ex != null) | |
| { | |
| Debug.LogMessage(LogEventLevel.Error, this, | |
| "Error changing scenario to '{0}': {1}", scenario.Key, ex); | |
| } | |
| }, TaskContinuationOptions.OnlyOnFaulted); |
| { | ||
| Debug.LogMessage(LogEventLevel.Debug, this, "Unable to find partition with key: '{0}'", partitionState.PartitionKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
There appears to be trailing whitespace after the closing brace on this line. Please remove the extra spaces to avoid noisy diffs and keep formatting consistent.
Suggested change
| } | |
| } |
Contributor
|
@AECohn Can you post a config snippet for the room combiner from the config that was running when you saw this issue? The way this should work is that the default scenario has a set of partitions configured that then have their state set based on the selected scenario, then the partition feedback(s) update with their manually-set state, and that then triggers the DetermineRoomCombinationScenario which calls the ChangeScenario function. My concern is that calling ChangeScenario as you've added could cause a runway loop. It's possible that something is checking for the current scenario too early in the start up secquence, as the initial scenario, whether manual or automatic, isn't set until all devices have completed initialization, which doesn't happen until AFTER all of the activation steps. It's also possible that the configuration isn't setup correctly for the partitions. |
Contributor
Author
setting defaultToManulMode false and removing disableAutoMode fixed it, but I had to update Essentials to get this config to render |
Contributor
|
Is that the whole config for the combiner? It doesn't seem to be a complete object. |
Contributor
Author
Here's the full object: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a small but important change to the
SetRoomCombinationScenariomethod in theEssentialsRoomCombinerclass. The update ensures that the scenario is activated immediately, preventing_currentScenariofrom being null when starting in manual mode.ChangeScenario(scenario)is called to activate the scenario and avoid a null_currentScenariowhen starting in manual mode.