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

Fixing GroupCategory enumeration #112

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Fixing GroupCategory enumeration #112

merged 2 commits into from
Jan 28, 2022

Conversation

justin-lovell
Copy link
Collaborator

Motivated by work in progress with PR #111. Communicating
breaking changes and configuring to change log and updating
behavior

@justin-lovell justin-lovell marked this pull request as draft January 15, 2022 13:27
@justin-lovell justin-lovell changed the title Fixing GroupCategory enumeration. WIP - Fixing GroupCategory enumeration. Jan 15, 2022
@justin-lovell
Copy link
Collaborator Author

This would take a while to fully implement because the implementation here is incorrect -- previously it would have only gotten the airplanes only, and not the other unit types.

.get_groups(Request::new(GetGroupsRequest {
coalition: coalition.into(),
category: None,
}))

I'm considering we change the StreamUnitsRequest to include the GroupCategory with the motivation of that the different units move at different speed, and hence it would result in a different poll rate requirement.

@justin-lovell justin-lovell changed the title WIP - Fixing GroupCategory enumeration. WIP - Fixing GroupCategory enumeration Jan 26, 2022
@justin-lovell justin-lovell changed the title WIP - Fixing GroupCategory enumeration Fixing GroupCategory enumeration Jan 26, 2022
@justin-lovell justin-lovell marked this pull request as ready for review January 26, 2022 08:36
src/rpc/mission.rs Outdated Show resolved Hide resolved
src/stream.rs Outdated Show resolved Hide resolved
src/rpc/mission.rs Outdated Show resolved Hide resolved
@rkusa
Copy link
Collaborator

rkusa commented Jan 26, 2022

If we add this filter, we'd also have to filter the units added in the following line (where it listens for birth events):

https://github.com/DCS-gRPC/rust-server/blob/main/src/stream.rs#L138

Motivated by work in progress with PR #111. Communicating
breaking changes and configuring to change log and updating
behavior
src/stream.rs Outdated Show resolved Hide resolved
rurounijones
rurounijones previously approved these changes Jan 27, 2022
rkusa
rkusa previously approved these changes Jan 27, 2022
Copy link
Collaborator

@rkusa rkusa left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@rurounijones rurounijones left a comment

Choose a reason for hiding this comment

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

@justin-lovell It looks, to me that there are 3 commits in this PR and the latter 2 might accidentally have not been squashed together.

Could you check on your end and squash them if they are supposed to be; or change the commit messages (They are currently identical) to be more specific to the commit goal if you think they should be separate commits.

Primary motivation is to ensure consistency with the underlying
`GetGroups` implementation.

Additional benefit is that different polling rates may be specified to
the different categories. This makes sense because jets fly faster than
ground units!
@justin-lovell
Copy link
Collaborator Author

@rurounijones -- yeah, the last one was my bad. I must have forgotten the amend.

Having the two commits is recording good history. One was for the linting, and the second one is to align StreamUnits up to standard.

@rurounijones rurounijones merged commit 30aaf2b into DCS-gRPC:main Jan 28, 2022
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.

3 participants