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 GetGroups, GetUnits and GetUnitPosition methods #11

Merged
merged 5 commits into from
Aug 23, 2021
Merged

Conversation

rkusa
Copy link
Collaborator

@rkusa rkusa commented Jul 9, 2021

First set of methods to support #8 eventually.

This PR adds the following methods:

  • GetGroups

    grpcurl.exe -plaintext -import-path ./protos -proto ./protos/dcs.proto -d '{\"coalition\": 2}' 127.0.0.1:50051 dcs.Mission/GetGroups
    
  • GetUnits

    grpcurl.exe -plaintext -import-path ./protos -proto ./protos/dcs.proto -d '{\"groupName\": \"Aerial-1\"}' 127.0.0.1:50051 dcs.Mission/GetUnits
    
  • GetUnitPosition

    grpcurl.exe -plaintext -import-path ./protos -proto ./protos/dcs.proto -d '{\"name\": \"Aerial-1-1\"}' 127.0.0.1:50051 dcs.Mission/GetUnitPosition
    
  • StreamUnits

    .\grpcurl.exe -plaintext -import-path ./protos -proto ./protos/dcs.proto -d '{}' 127.0.0.1:50051 dcs.Mission/StreamUnits
    

protos/common.proto Outdated Show resolved Hide resolved
protos/common.proto Outdated Show resolved Hide resolved
protos/group.proto Outdated Show resolved Hide resolved
@rkusa rkusa marked this pull request as ready for review July 9, 2021 12:41
@rkusa rkusa requested a review from rurounijones July 9, 2021 12:41
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.

Yaaay! Thanks for the changes 😁

You say in your comments that this is a pre-requisite for #8 but could you explain how? I think there is some rustiness nuance bit here that I am missing.

My current understanding is that gRPC methods are purely called by network clients but are you planning on calling these from inside the Rust server itself to implement the streaming or something along those lines?

lua/methods/group.lua Show resolved Hide resolved
protos/common.proto Outdated Show resolved Hide resolved
protos/common.proto Outdated Show resolved Hide resolved
protos/group.proto Outdated Show resolved Hide resolved
@rkusa
Copy link
Collaborator Author

rkusa commented Jul 10, 2021

Thanks for the feedback!

You say in your comments that this is a pre-requisite for #8 but could you explain how? I think there is some rustiness nuance bit here that I am missing.

My current understanding is that gRPC methods are purely called by network clients but are you planning on calling these from inside the Rust server itself to implement the streaming or something along those lines?

I am not sure if I am going to re-use the gRPC methods directly, but I am definitely planning on re-using the Lua part and maybe the gRPC request and response structs. The rough plan I have in the back of my head (not thought out completely yet, and might still change):

  • Create a new gRPC service with a method to get all alive units. I am not sure about the name yet, but I am thinking about naming the service in a way that makes it clear that this is not simply forwarding calls to the mission env and instead does some extra internal stuff. Something in the direction of dcs.Extra/GetUnits (I don't like this name, but should show the direction I am thinking about).
  • Once this method is first called, it does an initial sync using self.request("getGroups", request).await and self.request("getUnits", request).await
  • After the initial sync:
    • it always responds with its internal copy of alive units when dcs.Extra/GetUnits is called
    • it tries to keep its internal copy up to date by listening to events via self.ipc.events().await
    • since the above is not the most reliable method in DCS, it does some extra checks per unit on a regular basis

Does this make sense to you, or do you see any issues or is it maybe even going into the wrong direction for what you were aiming for in #8?

@rurounijones
Copy link
Contributor

rurounijones commented Jul 22, 2021

Ok, so I think I understand what you are going for but I am not sure how useful it would be to a client to have units without the curent position information. I like the idea of having a separate copy in rust so we do not need to call into the mission env but I don't think that is feasible given how I see clients using this.

My vision is that the client calls Mission.StreamUnits or something like that and receives a constant stream of up-to-date unit objects (That includes their current position) with the same information that is present in the stream_events existing aPI.

The stream would either be individual unit objects sent at a fixed rate or a single array of all the units in the server every second (for example) or streaming an array of units in a group at a time .

This way the client just calls one API and gets a constant stream of updates. This removes the need for having to to have a separate "GetUnitPosition" API that the client would then need to call separately. In the use-cases I can think of, such as OverlordBot or a WebGCI page or an Arma Zeus like interface, the client would have to be calling the position update constantly anyway since it is a key part of the visualisation requirement.

The implications of this are that the Rust server needs to call into the mission environment to keep the unit positions up to date so maybe it is just as fast to call into the mission env and get all the unit information rather than keeping a separate copy around inside rust and keeping that updated with the current position. You could keep a separate copy around in rust and update it from the mission env and then send the rust copy in one big array at a fixed interval though.

In order to not stop-the-world by getting every unit in the game at once we could do something like 1 getGroup call then every $X_TIME we get all the units in a group and stream those units before moving on to the next group. I believe that is what LotATC does. To use some pseudocode here

while shutdown == false
  groups = missionenv.getGroups(coalition)
  groups.each do |group|
    units = missionenv.getUnitsInGroup(group)
    streamUnitsToClient(units) # Or CopyToRustMemory and the streaming is done elsewhere
    sleep(X)
  end
end  

The X could be configurable such that we should cycle through every group in a 2 second period so 2 / groupCount or something like that.

Regarding a comment elsewhere about geting other unit attributes (health, ammo etc.) I agree that this can be a separate call (or calls) or we could add it to the response object at a later date if we think we can do it without impacting performance.

@rkusa
Copy link
Collaborator Author

rkusa commented Jul 29, 2021

Rebased onto main (due to merge conflicts). I'll add an initial version of the unit streaming before moving the PR out of its draft state again

@rkusa rkusa marked this pull request as draft July 29, 2021 15:51
lua/exporters/object.lua Outdated Show resolved Hide resolved
lua/grpc.lua Show resolved Hide resolved
lua/methods/unit.lua Outdated Show resolved Hide resolved
protos/dcs.proto Show resolved Hide resolved
src/stream.rs Show resolved Hide resolved
}

/// The state of an active units stream.
// TODO: re-use one state for all concurrent unit streams?
Copy link
Collaborator Author

@rkusa rkusa Jul 29, 2021

Choose a reason for hiding this comment

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

I tend towards keeping a separate state per unit stream to not throttle another stream because one of the clients is very slow in consuming its data.

If we however think that re-using the state/stream is useful, we could make https://github.com/DCS-gRPC/rust-server/pull/11/files#diff-5cdcc3261dd77eadb37bcd60c5f85affcc9306454ae7bb8128e6d096ae51a631R84 a mpmc (multi-producer, multi-consumer) channel instead (it is currently a multi-producer, single-consumer channel) and clone the receiving end for each incoming gRPC request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend towards keeping a separate state per unit stream to not throttle another stream because one of the clients is very slow in consuming its data.

Are there any advantages to having an mpmc stream? Would it result in less load on the server or something like that? I agree with yout bias of not letting one client screw the others via an accidental "slowloris" attack.

When I wrote tac_scribe, that reads tacview events, I made sure that one thread's job was to receive messages and put them on an internal memory queue for actual processing. This made sure that the receiving thread was fast and would not cause any slowdown.

If there are serious advantages to having mpcp stream then I think we might consider using it and documenting the crap out of the stream system and make sure clients do something similar to tac_scribe but ultimately I am not too fussed either way and defer to you on this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I gave it some addition thought and I think the question comes down to:

a) Should we expose unit-stream-instance-specific settings (see #11 (comment)) or
b) not (= shared state).

When not (b), we can re-use the same state and reduce the amount of calls made to the mission environment in case of concurrent unit streams. I think I could also make it so that one client cannot negatively affect each other (by handling updates very slow), but I'd probably have to buffer updates for each clients and thus terminate them if their respective buffer gets full.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am all for the option that reduces the number of calls made into the mission environment (Which I think is priority #1) and I don't see a problem with re-using the state.

I think the idea of having a buffer and auto-termination is a good idea. One potential issue I can see with auto-termination is that the queue could get instantly full on the initial unit dump as the client connects and therefore the client gets instantly disconnected? Not sure.

@rurounijones
Copy link
Contributor

rurounijones commented Aug 2, 2021

Tested over the weekend on a light mission with units getting spawned and destroyed. Seemed to work without any issues.

Is there any way we can measure time spent in lua or something like that to reassure server owners that it is not very impactful? (Either some test or via logging performance metrics like tacview does?)

Jupiter2

@rurounijones
Copy link
Contributor

rurounijones commented Aug 2, 2021

After doing some more UI work there are some things we need to think about how to do.

Group ID

There is no way to associate a unit with a group from unit streaming. This is because the Unit does not include the group information or even a group Id.

This makes streaming much more difficult (especially f you are trying to build a treeview for units for example)

Include the Group in the unit

We could always include the group object (ID and name only, not the units) (using getGroup() on the unit) or, if we have the info available, only include the group the first time a unit is seen.

Include the Group ID in the unit

Only include the groupId in the Unit object. This will give us enough information to create a group in our UI with a unique ID and the rest of the information for that group can be included in a GetGroups call.

Include the group ID and the group name

Include the group ID and the group name in the Unit so that we have all the information we need for building a UI up-front. (My least favourite option)

Stream Group objects with name and ID only.

Stream Group objects along with Unit and UnitGone. The group would need to be streamed the first time it is seen by the client

Stream group objects with everything upon first load.

The first time a new group is seen then stream the group object with all its associated units. This would require an internal rust list of "known" groups per client so that it knows what to send


Not sure what solution is the best here. They all have their pros and cons. What do yout think? I think just including the GroupId in the unit object is probably the easiest and most consistent-with-other-things option.

I did the modifications locally and got the following. The idea is that a "new group" is created each time a unit appears with a new group Id. And then in another thread we will call GetGroups on a timer to then populate the name of the group separately.

image

Here is a screenshot of the tree with a after groups have been updated (which is done on a timer every 10 seconds currently).

image

@rkusa
Copy link
Collaborator Author

rkusa commented Aug 22, 2021

Not sure what solution is the best here. They all have their pros and cons. What do yout think? I think just including the GroupId in the unit object is probably the easiest and most consistent-with-other-things option.

I did the modifications locally and got the following. The idea is that a "new group" is created each time a unit appears with a new group Id. And then in another thread we will call GetGroups on a timer to then populate the name of the group separately.

I'd probably go for including the group name for each unit. I tend towards the name because there is a getByName, but there is no getByID. Wdyt about name instead of id?

Stream Group objects with name and ID only.

Stream Group objects along with Unit and UnitGone. The group would need to be streamed the first time it is seen by the client

This would also sound good to me. Any preference between the two after working on the UI shown above?

@rurounijones
Copy link
Contributor

rurounijones commented Aug 22, 2021

Whoops, yes, GroupName is the better option.

If we are using GroupName instead of the GroupId in the unit then the first unit that appears with a so-far-unseen group name can automatically create the group as well.

This means that streaming Group objects becomes unecessary for the purposes of the UI I posted above. I am trying to think of any scenario where streaming group objects would be beneficial but cannot think of anything right now. Since GroupNames have to be unique anyway it isn't like we are losing anything by not having an ID.

However if streaming Group objects when they are first created is easy enough then we should probably do it just to be future-proof.

@rkusa
Copy link
Collaborator Author

rkusa commented Aug 23, 2021

Rebased due to merge conflicts 🔀

@rkusa rkusa marked this pull request as ready for review August 23, 2021 07:37
@rkusa
Copy link
Collaborator Author

rkusa commented Aug 23, 2021

We've decided to merge the PR as is and create follow-ups for the remaining open points

@rkusa rkusa merged commit b040017 into main Aug 23, 2021
@rkusa rkusa deleted the group-unit-methods branch August 23, 2021 07:42
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