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

Eval placeholder args in show update method (from #1702) #1750

Merged

Conversation

avanwinkle
Copy link
Collaborator

This PR reconciles a gap caused by #1557 which added support for updating a running show, and #1702 which added support for dynamic speed values in shows.

The change by @cobra18t in #1702 changed the expected type of the speed parameter from a float to a PlaceholderTemplate, which caused a crash in the update method from #1557 that was not expecting a placeholder object.

The resolution here is tricky because in a show, once the template is evaluated it's the resulting value that's saved to the show, not the placeholder template—otherwise the template would be re-evaluated every step of the show, costing overhead. This complicates the update call, however, which that compares the two values prior to generating the updated config to avoid rewriting a show that hasn't changed. Since the incoming value is a placeholder template and the current value is a float, the comparison is not supported.

The fix in this PR is to evaluate the new speed prior to the update call, and to remove the value comparison. The downside is that a call to update speed will always generate a new show config even if the speed is unchanged, but the upside is that update calls work with placeholder templates instead of crashing.

go faster faster

Copy link

sonarcloud bot commented Feb 4, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@avanwinkle avanwinkle merged commit 0072880 into missionpinball:dev Feb 22, 2024
14 checks passed
@avanwinkle avanwinkle deleted the show-update-placeholder-args branch February 22, 2024 00:48
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.

1 participant