Skip to content

Set terrain arg to store true. #374

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.

Sign up for GitHub

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

Jump to bottom
Merged
louis-e merged 2 commits into from
Apr 12, 2025
Merged

Set terrain arg to store true. #374

louis-e merged 2 commits into from
Apr 12, 2025

Conversation

@benjamin051000
Copy link
Contributor

@benjamin051000 benjamin051000 commented Feb 19, 2025

This was erroneously set to store false, which means it's impossible to enable the terrain from the cli.

@benjamin051000
Copy link
Contributor Author

benjamin051000 commented Feb 19, 2025

Hm, this is failing with the following:

cargo r --no-default-features --release -- --bbox "-71.035194,42.327427,-71.008286,42.342210" --path "/home/benjamin/.var/app/org.prismlauncher.PrismLauncher/data/PrismLauncher/instances/Fabulously Optimized(3)/.minecraft/saves/testing" --terrain

(with a dbg! around the first line of ground::fetch_elevation_data) Prints:

[src/ground.rs:130:32] bbox_str.split_whitespace().map(|s: &str|
s.parse::<f64>()).collect::<Result<Vec<f64>, _>>() = Err(
    ParseFloatError {
        kind: Invalid,
    },
)
Warning: Failed to fetch elevation data: invalid float literal

@benjamin051000 benjamin051000 mentioned this pull request Feb 19, 2025
Closed
@benjamin051000
Copy link
Contributor Author

benjamin051000 commented Feb 19, 2025

I'll add a unit test that verifies this before merging.

@benjamin051000 benjamin051000 force-pushed the fix-terrain-flag branch 11 times, most recently from 5bdea24 to afdfc0a Compare February 22, 2025 04:09
@benjamin051000
Copy link
Contributor Author

benjamin051000 commented Feb 22, 2025 •
edited
Loading

Blocked on #378

@benjamin051000 benjamin051000 mentioned this pull request Mar 9, 2025
Merged
@benjamin051000 benjamin051000 force-pushed the fix-terrain-flag branch 2 times, most recently from c25cb5f to 358ad53 Compare April 10, 2025 20:16
@benjamin051000
Set terrain arg to SetTrue.
1749f6a 
This was erroneously set to `SetFalse`, which means it's impossible to
enable the terrain from the cli.

According to clap docs, the default action for a bool arg is SetTrue,
and the default value is false. (https://docs.rs/clap/latest/clap/_derive/index.html#arg-types)

So we don't need to provide any additional args here.
@benjamin051000
Add unit tests for --winter, --debug, --terrain flags.
6429280
@benjamin051000
Copy link
Contributor Author

benjamin051000 commented Apr 10, 2025

@louis-e once CI passes, this is ready for signoff

@benjamin051000 benjamin051000 mentioned this pull request Apr 10, 2025
Merged
@louis-e
Copy link
Owner

louis-e commented Apr 12, 2025

So I guess we're starting with unit tests now ;)
Thanks a lot :)

@louis-e louis-e merged commit 2a1233c into louis-e:main Apr 12, 2025
3 checks passed
@benjamin051000 benjamin051000 deleted the fix-terrain-flag branch April 12, 2025 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@benjamin051000 @louis-e