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

feature: support relativePosition.radius in subscriptions and REST requests #446

Merged
merged 5 commits into from
Feb 16, 2018

Conversation

sbender9
Copy link
Member

@sbender9 sbender9 commented Feb 7, 2018

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:

{
  context: {
    radius: 10,
    position: {
      longitude: -76.4639314,
      latitude: 39.0700403
    }
  },
  subscribe: [
    {
      path: 'navigation.*'
    }
  ]
}

req.query.radius
)
) {
debug(`vessel: ${key} ${JSON.stringify(position)}`)
Copy link
Member

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(...)

@@ -228,3 +238,16 @@ function normalizeUpdate (update) {
}
})
}

function updatePosition (delta, spark) {
Copy link
Member

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?

if (!_.isUndefined(msg.context.relativePosition)) {
debug('setting up to store position')
spark.request.position = {}
msg.context.relativePosition.position = spark.request.position
Copy link
Member

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.

return x => false
}
return normalizedDeltaData => {
return checkPosition(app, relativePosition, normalizedDeltaData)
Copy link
Member

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?

Copy link
Member Author

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

@sbender9
Copy link
Member Author

I think the only thing left here is the naming. One option for the subscription:

{
          context: {
              contexts: "vessels.*",
              radius: 10,
              position: {
                longitude: -76.4639314,
                latitude: 39.0700403
              }
            }
          },
          subscribe: [
            {
              path: '*'
            }
          ]
        }

@sbender9
Copy link
Member Author

Or just

{
  context: {
    radius: 1,
    position: {
      longitude: -76.4639314,
      latitude: 39.0700403
    }
  },
  subscribe: [
    {
      path: 'navigation.position'
    }
  ]
}

@tkurki tkurki merged commit 66f66d1 into master Feb 16, 2018
@tkurki tkurki deleted the relative-position branch February 16, 2018 06:36
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