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

Add missing fields and change register methods #207

Merged
merged 5 commits into from
Jun 7, 2024

Conversation

lukaszzborek
Copy link
Contributor

@lukaszzborek lukaszzborek commented May 30, 2024

Breaking:

Other:

  • Add missing rain related fields
  • Add method to get weather forecast from schedule (GetSeason)

- Change AddIRacingDataApi methods to return IHttpClientBuilder instead of IServiceCollection

Other:
- Add missing rain related fields
- Add method to get weather forecast from schedule (GetSeason)
Copy link
Owner

@AdrianJSClark AdrianJSClark left a comment

Choose a reason for hiding this comment

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

The bulk of these changes are great. Thanks very much for the contribution!

I'm just a bit concerned merging the weather forecast when the values are so unfriendly. If we can work on that class so that people using it don't have to have so much knowledge of how the values are returned I think that would be much more useful.

Due to the breaking change included, I think this change will have to wait and be included with the Season 3 milestone release in a couple of weeks. If there are any particular parts you want merged and released before then, please open a smaller PR and let me know.

src/Aydsko.iRacingData/IDataClient.cs Outdated Show resolved Hide resolved
src/Aydsko.iRacingData/Series/WeatherForecast.cs Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Many of the properties here need a converter. Their values appear to be converted to avoid decimal places. An example is the AirTemp property which may have a value like 2410 in the response, however int he iRacing UI that will be displayed as 24°C. This I assume is dividing by 100 then rounding to the nearest whole number. A value of 1954 will be displayed as 20°C (1954 / 100 = 19.54 which rounded to the nearest whole number is 20).

I think the full list of properties which will require this conversion are:

  • AirTemp
  • RawAirTemp
  • RelHumidity
  • PrecipitationChance

The CloudCover field appears to use a different conversion. It needs to be divided by 10 instead. So a value of 113 will become 11.3% or, as shown in the iRacing UI, 11% (rounded down).

I can't work out what units the WindSpeed or WindDir are in. The WindDir value doesn't convert directly to the WindDirection enumeration (unfortunately). Looking at the sample file you used in the test and some of my own data I think that WindDir could be in degrees. Meaning 0 = North, 180 = South. I'll have to check on the forums though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the types and added converters.
I also saw your comments in the iRacing forum, so I will wait for your answer. My analysis of wind speed indicates it is measured in meters per second, which is a bit strange because this unit is not used elsewhere (in lookup I see only mph and kph). When I convert it to km/h, it's nearly the same as in the UI in the weather summary (not the forecast). I also convert the wind direction from degrees to a rounded direction.

Copy link
Owner

Choose a reason for hiding this comment

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

Jason got back with some fantastic detail on the thread: https://forums.iracing.com/discussion/comment/541516/#Comment_541516

If you're able to update things that would be great.

Just let me know if you would like me to take care of it. I'll probably be working on this over the weekend as I'm racing and live streaming tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, I will correct it today, but most of it is already done.

Copy link
Contributor Author

@lukaszzborek lukaszzborek Jun 6, 2024

Choose a reason for hiding this comment

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

I think is done, added comments for all properties and fix converters

@AdrianJSClark
Copy link
Owner

Ignore the failing build. That's due to permissions I've not managed to work out for external people submitting pull requests. Sorry about that.

@lukaszzborek
Copy link
Contributor Author

The bulk of these changes are great. Thanks very much for the contribution!

I'm just a bit concerned merging the weather forecast when the values are so unfriendly. If we can work on that class so that people using it don't have to have so much knowledge of how the values are returned I think that would be much more useful.

Due to the breaking change included, I think this change will have to wait and be included with the Season 3 milestone release in a couple of weeks. If there are any particular parts you want merged and released before then, please open a smaller PR and let me know.

Yes, it is a good idea to wait for the new season with the breaking changes. Other things can also wait. Currently, I am using my own build in the app.

@AdrianJSClark AdrianJSClark linked an issue Jun 2, 2024 that may be closed by this pull request
@AdrianJSClark AdrianJSClark merged commit a5ce56f into AdrianJSClark:main Jun 7, 2024
@AdrianJSClark
Copy link
Owner

Thanks for the contribution, @lukaszzborek!! Awesome stuff which is much appreciated.

@lukaszzborek lukaszzborek deleted the rain-things branch September 6, 2024 21:08
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.

Change "AddIRacingDataApi" to return "IHttpClientBuilder"
2 participants