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

Issue 82 oo rwrite #84

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Issue 82 oo rwrite #84

wants to merge 13 commits into from

Conversation

mill-j
Copy link
Collaborator

@mill-j mill-j commented Mar 8, 2021

This branch is for the development of an object oriented fgcom-mumble plugin using https://github.com/mumble-voip/mumble-plugin-cpp. Still very much work in progress. The aim is to create a drop-in replacement for the existing plugin.

What currently works:

  • Receiving data from RadioGUI
  • Transmitting audio/udp data to existing fgcom-plugin mumble users using RadioGUI
  • Receiving audio/udp data from existing fgcom-plugin mumble users

What needs to be done:

  • Basically all incoming transmittion checks, such as frequency, distance, range, etc.
  • Conversions such as MSL to AGL, ft to m.
  • Lots of bug testing

ToDo list:

  • Basic OO mumble plugin structure
  • port UDP server data interface ("UDP server")
  • port internal data structures to classes/objects
    • Identity support for local and remote state
    • Radio support for the identities
  • port plugin-io (sending and receiving notifications trough mumble plugin framework)
  • port garbage collector
  • port UDP client data interface ("UDP client" for RDF -> ATC-Pie)
  • port updater
  • port test tools (if neccesary)
  • implement unit tests (Add unit tests for QA #85)

References:

@hbeni hbeni linked an issue Mar 8, 2021 that may be closed by this pull request
@hbeni
Copy link
Owner

hbeni commented Mar 8, 2021

Cool, thanks!

And don’t feel obligated to complete this.
This will be alot(!) of work duplicating already done stuff just for the benefit of being OO.
The current C-style code centered around the idea of „modules“ and (i hope) easy enough to maintain.

@hbeni hbeni linked an issue Mar 8, 2021 that may be closed by this pull request
5 tasks
@hbeni
Copy link
Owner

hbeni commented Mar 8, 2021

I added some references to the documentation and a basic todo list with tickable items to the inital comment above. This way, we can easily track the progress on a higher level.

Btw. I have actually spend some toughts onto the structure for internal and remote state.
I really think probably we want some "meta class" holding the remote and the local state. That class could provide the basic and high-level methods for manipulating state (add/remove Identites/radios; see if a given identity is withing range of another, etc). That can then easily be hooked up in the mumble plugin callback for processing the audio.
Then the idea to plumb the radio-model for a given radio-instance is a really great one i think.

Another point i have seen: You already provide some methods for generating a UDP string for some values (like location).
What do you think about the convention to supply the classes with a std::string toString() method like Java does, and thus get a somewhat wider known standard method name?
Also, what about the idea to implement an abstract packet parser class which deals with all the packing/unpacking of UDP and plugin-io data fields (this would make the UDP server, UDP client and internal plugin-io more abstract, and especially there I currently have some code duplication and spaghetti code)?

Btw. as we go along: so you have experience with coding unit tests? This would be a great addition too, and i think with the OO approach this would be more easy to code up (new: #85).
💡 Adding a top-level class for all the state stuff etc would also allow us to implement plugin-framework independent unit tests, because we decouple mumble's plugin callbacks from the actual plugin code payload

@hbeni hbeni added the mumble-plugin Affecting mumble plugin label Mar 8, 2021
@mill-j
Copy link
Collaborator Author

mill-j commented Mar 8, 2021

@hbeni

Ok thanks.

I do like the idea of adding a class for storing and managing fgcom_identity objects. Any idea of a name for the class?

I'm not sure what you mean by:

	What do you think about the convention to supply the classes with a std::string toString() method like Java does, and thus get a somewhat wider known standard method name?

I'm not familiar with Java, the reason fgcom_identity::getUdpMsg() returns std::vector is that's what the m_api.sendData takes as an argument, so there is no need to convert from string to std::vector every time you send a message via sendData(). That said, I might be completely misunderstanding what you meant.

A class to parse udp data would allow us to remove even more code from the main class. And it could be used in both onReceiveData() and fgcom_spawnUDPServer().

I have no expirence with coding unit tests, however this project is for learning experience so we might need to check into that.

As far as the completetion of this branch: I'll continue to work on it when I have time, but since I generally have less time for programming during spring and summer, don't expect it to be done very fast :D

@hbeni
Copy link
Owner

hbeni commented Mar 8, 2021

I do like the idea of adding a class for storing and managing fgcom_identity objects. Any idea of a name for the class?

Anything with "state" in it, maybe StateManager or StateStorage or something like that?

I'm not familiar with Java, the reason fgcom_identity::getUdpMsg() returns std::vector is that's what the m_api.sendData takes as an argument, so there is no need to convert from string to std::vector every time you send a message via sendData(). That said, I might be completely misunderstanding what you meant.

I think i need to read up the API then :)

I have no expirence with coding unit tests, however this project is for learning experience so we might need to check into that.

Currently tinkering with that and i think it's somewhat easy. I aleady added a branch for that.

As far as the completetion of this branch: I'll continue to work on it when I have time, but since I generally have less time for programming during spring and summer, don't expect it to be done very fast :D

No rush! We have no pressure here, as the current codebase is working already.

@hbeni hbeni added this to the 1.0 milestone Mar 13, 2021
@hbeni hbeni force-pushed the master branch 3 times, most recently from c4de535 to fe74a79 Compare May 18, 2021 14:38
@mill-j
Copy link
Collaborator Author

mill-j commented May 18, 2021

@hbeni I haven't really been working on this much recently. It's spring/summer and I'm very much an outdoor person. I've been watching most of the commits and issues and it appears that very few are bug fixes to the actual code anymore, which suggests that the codebase itself is fairly stable anymore. I'm willing to keep working on it if you think it's worth it. It just won't be very fast :) If you want to delete these branches and put a won't fix on the linked issues, feel free to do that as well.

@hbeni
Copy link
Owner

hbeni commented May 18, 2021

Hi there, it all depends if you want to continue to pursue it.
If it doesnt make fun anymore, in think it is not necessary to finish - I share your opinion tough that the codebase looks halfway stable and somewhat matured.

I think beeing fully OO would benefit in the long term; however it’s already alot(!) of work to even get close to what is here already - all with the constant danger of introducing new bugs along the way.
For the code alone i don’t think it is worth the hassle, just maybe if the migration is fun to Someone.

Maybe a compromise is to merge the other branch into this here and just keep this one. Deleting the work already done is neither necessary nor fun

Initial fgcom_stateManger implemetation.
@mill-j
Copy link
Collaborator Author

mill-j commented May 18, 2021

Maybe a compromise is to merge the other branch into this here and just keep this one. Deleting the work already done is neither necessary nor fun

Well I learn quite a bit about threading and udp so I think it was certainly worth it on my part. However as you say, the amount of work that still needs done is considerable since so far all the work has been getting audio both ways. I'm fine with leaving this here, in case me, you, or somebody else needs somthing to work on later, This should be a great base or at least an example for using the OO plugin framework. I do have more commits with improvements on this such as supporting multiple local clients, however I still haven't got audio working reliably both ways with the current plugin for some unknown reason, which is why I still haven't pushed. I'll hold on the them and maybe find time to fix that.

@hbeni hbeni removed this from the 1.0 milestone May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mumble-plugin Affecting mumble plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to cmake and mumble-plugin-cpp, also OO rewrite Refactor code based on review
2 participants