-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
feature: support relativePosition.radius in subscriptions and REST requests #446
Conversation
req.query.radius | ||
) | ||
) { | ||
debug(`vessel: ${key} ${JSON.stringify(position)}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor here, but as discussed in canboatjs JSON.stringify is surprisingly heavy and I'd like to start the habit of doing this like debug.enabled && debug(...)
lib/interfaces/ws.js
Outdated
@@ -228,3 +238,16 @@ function normalizeUpdate (update) { | |||
} | |||
}) | |||
} | |||
|
|||
function updatePosition (delta, spark) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change this to updatePosition(delta, position)
as this doesn't need to know anything about spark.request?
lib/interfaces/ws.js
Outdated
if (!_.isUndefined(msg.context.relativePosition)) { | ||
debug('setting up to store position') | ||
spark.request.position = {} | ||
msg.context.relativePosition.position = spark.request.position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relativePosition
is not very clear, how about msg.context.subscriber.position
? Ditto for spark.request.subscriberPosition
.
lib/subscriptionmanager.js
Outdated
return x => false | ||
} | ||
return normalizedDeltaData => { | ||
return checkPosition(app, relativePosition, normalizedDeltaData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't checkPosition here get the relativePosition from the subscribeCommand, which is static and will never change? So we are updating the subscriber's position in spark.request, but that is never used in checkPosition?
Thinking this through how useful is the updating current position going to be in real life? It only has the lifetime of the ws connection and can be easily updated by hanging off the ws conenction and recconnecting every x hours. For this to be useful we would need to be dealing with orbital speeds or very small radii. What do you think? Ditch or keep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. You’re right. I’ll ditch it
I think the only thing left here is the naming. One option for the subscription:
|
Or just
|
This feature allows ws subscriptions to be based on the relative position of the vessel. This allows the client to to only get information about other vessels that are within a specific distance. For example: