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

First exchange of Vircadia protocol packets with the Vircadia Web SDK. #1259

Merged
merged 14 commits into from
Jul 16, 2021

Conversation

ctrlaltdavid
Copy link
Collaborator

@ctrlaltdavid ctrlaltdavid commented Jun 26, 2021

This PR in conjunction with Vircadia-Web-SDK PR vircadia/vircadia-web-sdk#9 implements the first exchange of Vircadia protocol packets with the domain server.

Merge sequence:

@ctrlaltdavid ctrlaltdavid marked this pull request as ready for review July 1, 2021 01:31
@ctrlaltdavid ctrlaltdavid added enhancement New feature or request needs CR (code review) needs testing (QA) The PR is ready for testing labels Jul 1, 2021
libraries/networking/src/AddressManager.h Outdated Show resolved Hide resolved
libraries/networking/src/LimitedNodeList.cpp Show resolved Hide resolved
case SocketType::UDP:
// WEBRTC TODO: The Qt documentation says that the following call shouldn't be used if the UDP socket is connected!!!
// https://doc.qt.io/qt-5/qudpsocket.html#writeDatagram
return _udpSocket.writeDatagram(datagram, sockAddr.getAddress(), sockAddr.getPort());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use writeDatagram anyway, against the recommendation, then? It seems easy to call write here as recommended.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the way it's currently done in master for UDP packets and should be addressed there rather than here.
#1279



/// @brief Multiplexes a QUdpSocket and a WebRTCSocket so that they appear as a single QUdpSocket-style socket.
class NetworkSocket : public QObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps this should inherit from QAbstractSocket instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

QAbstractSocket inherits from QIODevice and no code from either of these is needed. It's not clear to me whether inheriting from QAbstractSocket would be beneficial or just overhead and a potential source of problems.



/// @brief Provides a QUdpSocket-style interface for using WebRTCDataChannels.
class WebRTCSocket : public QObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it should inherit from QAbstractSocket?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ditto.

tools/doxygen/Doxyfile Outdated Show resolved Hide resolved
tools/doxygen/Doxyfile Outdated Show resolved Hide resolved
tools/doxygen/README.md Outdated Show resolved Hide resolved
@digisomni digisomni added the web sdk Related to the Vircadia Web SDK label Jul 5, 2021
@digisomni digisomni self-requested a review July 8, 2021 21:36
Web Part 1: Interface + SDK - May through October 2021 automation moved this from Review in progress to Reviewer approved Jul 13, 2021
@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. and removed needs CR (code review) labels Jul 15, 2021
Copy link
Member

@digisomni digisomni left a comment

Choose a reason for hiding this comment

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

Worked fine according to the unit tests on master of the Vircadia Web SDK as of 7/15/2021.

@digisomni digisomni merged commit dfd71ab into vircadia:webapp Jul 16, 2021
Web Part 1: Interface + SDK - May through October 2021 automation moved this from Reviewer approved to Done Jul 16, 2021
@ctrlaltdavid ctrlaltdavid deleted the dev/webrtc-packet branch July 21, 2021 00:08
@digisomni digisomni added the unmerged-dev A project being developed in parallel to master, to be merged at a later date. label Sep 7, 2021
@digisomni digisomni changed the title First exchange of Vircadia protocol packets with the Vircadia Web SDK First exchange of Vircadia protocol packets with the Vircadia Web SDK. Sep 7, 2021
@digisomni digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing enhancement New feature or request labels Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR Approved At least one code reviewer has approved the PR. QA Approved The PR has been tested successfully. unmerged-dev A project being developed in parallel to master, to be merged at a later date. web sdk Related to the Vircadia Web SDK
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants