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

Fixed game starting completely dark. #740

Merged
merged 1 commit into from
Nov 5, 2018

Conversation

maxrd2
Copy link
Contributor

@maxrd2 maxrd2 commented Nov 5, 2018

When starting game directly without ja2-launcher and no brightness setting defined in json or command line it would default to 0.0, and whole screen would turn completely dark.

I'd appreciate if someone with more experience in rust and serde_json would check this to confirm this is correct solution. Apparently serde(default) would set this value to 0.0 instead of using what was defined here: https://github.com/ja2-stracciatella/ja2-stracciatella/blob/master/rust/src/stracciatella.rs#L196

I thought serde(default) is just supposed to add line to json if not present using the default value from that line... apparently not.

Edit: this was introduced by changes from my pull request #734

@selaux
Copy link
Member

selaux commented Nov 5, 2018

I think the correct one would be #[serde(default = "1.0")]

@selaux
Copy link
Member

selaux commented Nov 5, 2018

@maxrd2
Copy link
Contributor Author

maxrd2 commented Nov 5, 2018

Have already looked there after trying #[serde(default = 1.0)] which doesn't work because it expects string-with-the-callback-function-name instead of default value.
The default value is already defined here: https://github.com/ja2-stracciatella/ja2-stracciatella/blob/master/rust/src/stracciatella.rs#L196, but it doesn't get picked up for some reason when #[serde(default)] is there.

The example at https://serde.rs/attr-default.html is using the function default_resource().

Updating the patch to use the function, which also fixes it, but have hoped there is something that looks cleaner, or that there is a way it could pick default brightness value directly from EngineOptions::Default::default()

@selaux
Copy link
Member

selaux commented Nov 5, 2018

Allright. I looked once more at the documentation but the following fact is more or less undocumented unfortunately (extracted it from this PR: serde-rs/serde#780).

The #[serde(default)] macro that the struct is annotated with already creates the behavior we want. Annotating the field again with #[serde(default)] will use the default value for the type in that field (in this case f64, which is 0.0). So simply removing the #[serde(default)] annotation from the brightness field gives us what we want.

@selaux
Copy link
Member

selaux commented Nov 5, 2018

@maxrd2
Copy link
Contributor Author

maxrd2 commented Nov 5, 2018

ok.. so how i had it fixed originally by removing #[serde(default)] was ok.
Will push that one again :)
Have added the #[serde(default)] originally because otherwise it didn't write brightness entry to json file when it's created for the first time.

Edit: actually it gets added with or without #[serde(default)]

@selaux
Copy link
Member

selaux commented Nov 5, 2018

Th for recognizing and the fix BTW. I created a PR for serde docs to clarify this: serde-rs/serde-rs.github.io#84

@selaux selaux merged commit e3bc4e0 into ja2-stracciatella:master Nov 5, 2018
@maxrd2 maxrd2 deleted the brightness-setting branch November 18, 2018 17:57
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