-
Notifications
You must be signed in to change notification settings - Fork 75
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
Implement login warp-in and improve translocate commands #294
Implement login warp-in and improve translocate commands #294
Conversation
Reviewer's Guide by SourceryThis pull request implements login warp-in functionality and improves the translocate command. The login warp-in feature moves players to a position 0.5 AU away from their last logout position upon login, while the translocate improvements allow for teleportation to various celestial objects in the solar system. File-Level Changes
Tips
|
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.
Hey @charles-m-knox - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -50,6 +52,36 @@ PyResult Command_translocate(Client* pClient, CommandDB* db, EVEServiceManager & | |||
return Command_tr(pClient,db,services,args); | |||
} | |||
|
|||
// `UpdateBubble` is a reusable function that synchronizes the player's position | |||
// and ensures that their bubble exists. | |||
static PyResult UpdateBubble(Client *pClient) { |
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.
suggestion: Evaluate the usage context of UpdateBubble
While the refactoring is good, consider if this function should be a member of the Client class instead of a static function in SystemCommands.cpp. This could improve encapsulation and make the code more maintainable.
PyResult Client::UpdateBubble() {
if (!IsInSpace())
throw CustomError("You're not in space.");
789ee92
to
ef7ff76
Compare
lgtm, love to see the work on Destiny! 🚀 |
This pull request brings in the following changes:
Details about the first change: Login warp-in
I tried to leave good comments in the code to describe everything that went into this, but I'll provide a bit of an explanation here as well.
Currently (before this PR), the behavior on in-space login that I see is that the universe is just a black void, or sometimes is an unpopulated and inaccessible solar system.
I was able to verify that the client is actually just out of sync with the server - you can actually just call
/sendstate
right after you log in, and the client is now synchronized with the server and you can do regular things./sendstate
essentially usesUpdateBubble()
to synchronize the client & server. I used this logic as the foundation for fixing the login warp functionality.As explained in the code, the new login warp cloaks and moves the player to a position 0.5 AU away from their last logout position.
UpdateBubble()
is called so that a bubble is created before the player's upcoming warp vector gets established.The client's state is set to
Login
and, on the next server tick, the actual DestinyWarpTo
method gets called, they're uncloaked, and manyBeyonce
player commands are restricted until the warp is completed. This prevents the user from aborting the warp preemptively, which is consistent with expected behavior.Finally, once the warp completes, the player's
LoginWarp
state is cleared out and they can use their ship again.To test this out, simply:
Known issues:
The only expected issue I ran into at this time is that sometimes the player's position isn't preserved on log out, so you might end up 0.5 AU away from 0.5 AU away from your original point. This is out of scope for this pull request, because the logout code isn't fully fleshed out, and it's probably related to that.
Details about the second change: Translocate improvements
I found that when translocating via the solar system menu to e.g. an asteroid belt or planet, the command failed due to an invalid lookup (the code isn't there yet).
To make it work, I implemented database queries against the
mapDenormalize
table to resolve celestials and use their spatial coordinates for translocation. It's not perfect - it will teleport you to the center of planets and moons (can be fixed later easily, I suspect) - but it's good enough for now. It also may not behave perfectly with existing translocation command permutations (fleet, other players, etc), but the number of testing permutations is significant enough such that I cannot realistically test them all.I bundled in the translocation command fixes with the warp-on-login changes because if you translocate in the middle of a login warp bubble before it collapses, the "warp complete" event never triggers, and thus the newly added login warp state values are never reset - meaning you will never be able to do anything with your ship for the whole client session (probably). This is an edge case that can only ever happen due to a translocation command.
Another edge case: If you translocate to a solar system (typically doable from the star map), translocating to 0,0,0 was buggy, so instead I query for the celestial that has the smallest
x
value for simplicity.To test this:
x
value and not the star.Thanks for reading/reviewing!
Summary by Sourcery
Introduce a login warp-in feature that synchronizes the client and server upon player login, moving the player to a safe distance and restricting actions until warp completion. Enhance the translocate command to work with celestial objects by resolving their spatial coordinates, improving the overall user experience when navigating through space.
New Features:
Enhancements: