Use double for destiny m_targetDistance instead of uint32 #295
Conversation
…alting after warp stop for slightly better stability
charles-m-knox
changed the title
|
Ooh nice catch! It would certainly make sense that this is the case. It's great to see someone going through the spiderweb of code which is Destiny 😁 Out of curiosity, do you think we have a similar problem with mass calculations? I know that the desync problems get significantly worse if you fly a capital ship around, and when flying a titan, it completely breaks. |
@jdhirst I'm not sure - I'd have to look into it! But now that you've mentioned it, I'll be on the lookout for it. |
… logging improvements and fixes
|
When a player is mid-warp (warp cruise), they're actually not in a sys bubble at all. This causes a few issues and unfortunately is inconsistent with the explanation of Because it is null in space, My latest commit 58dcf4e updates the I'll continue testing this over the next few days but I'm hopeful that this will be "stable" soon, and I'll open this PR up for proper review 🙂 Other changes in the commits on this branch are pretty much code formatting improvements. |
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - checking if in space"); | ||
| if (!pClient->IsInSpace()) { | ||
| throw CustomError ("You're not in space."); | ||
| if (pClient->GetShipSE()->DestinyMgr() == nullptr) | ||
| } | ||
|
|
||
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - checking if destiny manager is null"); | ||
| if (pClient->GetShipSE()->DestinyMgr() == nullptr) { | ||
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - destiny manager is null, setting to null origin"); | ||
| pClient->SetDestiny(NULL_ORIGIN); | ||
| if (pClient->GetShipSE()->SysBubble() == nullptr) | ||
| } | ||
|
|
||
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - checking if bubble is null"); | ||
| if (pClient->GetShipSE()->SysBubble() == nullptr) { | ||
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - bubble is null, setting system id"); | ||
| pClient->EnterSystem(pClient->GetSystemID()); | ||
| } | ||
|
|
||
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - checking if session is changing"); | ||
| if (pClient->IsSessionChange()) { | ||
| pClient->SendInfoModalMsg("Session Change Active. Wait %u seconds before issuing another command.", | ||
| pClient->GetSessionChangeTime()); | ||
| return new PyString("SessionChange Active. Request Denied."); | ||
| pClient->SendInfoModalMsg( | ||
| "Session Change Active. Wait %u seconds before issuing another command.", | ||
| pClient->GetSessionChangeTime() | ||
| ); | ||
|
|
||
| return new PyString("SessionChange Active. Request Denied."); | ||
| } | ||
|
|
||
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - setting state sent to false"); | ||
| pClient->SetStateSent(false); | ||
|
|
||
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - sending SetState"); | ||
| pClient->GetShipSE()->DestinyMgr()->SendSetState(); | ||
|
|
||
| _log(COMMAND__MESSAGE, "SystemCommands::SendState() - setting session timer"); | ||
| pClient->SetSessionTimer(); | ||
|
|
||
| return new PyString("Update sent."); | ||
| } |
There was a problem hiding this comment.
I've added a bit more verbosity to the logging for the sendstate command since it seems to solve some desync problems that I encounter with Destiny. It helps me figure out where errors are happening. If this is excessive, let me know, I can reduce it.
| if (is_log_enabled(DESTINY__WARP_TRACE)) { | ||
| _log( | ||
| DESTINY__WARP_TRACE, | ||
| "Destiny::WarpUpdate() %s(%u): Ship is %f from center of target bubble %u.", | ||
| mySE->GetName(), | ||
| mySE->GetID(), | ||
| m_targBubble->GetCenter().distance(m_position), | ||
| m_targBubble->GetID() | ||
| ); | ||
| } | ||
|
|
||
| if (m_targBubble->InBubble(m_position, true)) { | ||
| if (is_log_enabled(DESTINY__WARP_TRACE)) { | ||
| _log( | ||
| DESTINY__WARP_TRACE, | ||
| "Destiny::WarpUpdate() %s(%u): Ship at %.2f,%.2f,%.2f is calling Add() for bubble %u.", | ||
| mySE->GetName(), | ||
| mySE->GetID(), | ||
| m_position.x, | ||
| m_position.y, | ||
| m_position.z, | ||
| m_targBubble->GetID() | ||
| ); | ||
| } | ||
|
|
||
| m_targBubble->Add(mySE); | ||
|
|
||
| SetPosition(m_position, true); | ||
| } else { | ||
| _log( | ||
| DESTINY__WARP_TRACE, | ||
| "Destiny::WarpUpdate() %s(%u): adding to midWarpSystemBubble.", | ||
| mySE->GetName(), | ||
| mySE->GetID() | ||
| ); | ||
| SystemBubble* midWarpSystemBubble(sBubbleMgr.GetBubble(mySE->SystemMgr(), m_position)); | ||
| midWarpSystemBubble->Add(mySE); |
There was a problem hiding this comment.
Here is an actual code change. A sysbubble is created each server tick during warp, which fixes a few issues that arise because the ship is in space but before my code changes, it gets removed from the bubble it was in when starting warp, and its sysbubble attribute is a nullptr.
I have also tested this in a 2-person fleet warp. The other person should be in the same bubble (unable to verify due to noisy log output and no player ids in logs), but unfortunately, at this time the client does not show the other person once warp cruise has been reached. Maybe this can be addressed in the future.
| wt.entityID = mySE->GetID(); | ||
| wt.dest_x = m_targetPoint.x; | ||
| wt.dest_y = m_targetPoint.y; | ||
| wt.dest_z = m_targetPoint.z; | ||
| wt.distance = m_stopDistance; | ||
| wt.warpSpeed = GetWarpSpeed(); // warp speed x10 | ||
|
|
||
| updates.push_back(wt.Encode()); | ||
| //send a warp effect... | ||
|
|
||
| // send a warp effect... | ||
| OnSpecialFX10 sfx; | ||
| sfx.guid = "effects.Warping"; | ||
| sfx.entityID = mySE->GetID(); | ||
| sfx.isOffensive = false; | ||
| sfx.start = true; | ||
| sfx.active = true; | ||
| sfx.guid = "effects.Warping"; | ||
| sfx.entityID = mySE->GetID(); | ||
| sfx.isOffensive = false; | ||
| sfx.start = true; | ||
| sfx.active = true; | ||
|
|
There was a problem hiding this comment.
these are just code formatting changes
| if (!mySE->HasPilot()) { | ||
| return; | ||
| } | ||
|
|
||
| if (is_log_enabled(DESTINY__MESSAGE)) | ||
| _log(DESTINY__MESSAGE, "Destiny::SendSetState() Called for Ship:%s(%u) Pilot:%s(%u)", \ | ||
| mySE->GetName(), mySE->GetID(), mySE->GetPilot()->GetName(), mySE->GetPilot()->GetCharacterID()); | ||
| if (is_log_enabled(DESTINY__MESSAGE)) { | ||
| _log( | ||
| DESTINY__MESSAGE, | ||
| "Destiny::SendSetState() Called for Ship:%s(%u) Pilot:%s(%u)", | ||
| mySE->GetName(), | ||
| mySE->GetID(), | ||
| mySE->GetPilot()->GetName(), | ||
| mySE->GetPilot()->GetCharacterID() | ||
| ); | ||
| } | ||
|
|
||
| // if the player is not warping, tell the client they're not warping. | ||
| // As of 2024-10-03, this doesn't always work and there are still issues | ||
| // with the client sometimes not starting a warp sequence. | ||
| std::vector<PyTuple*> updates; | ||
| OnSpecialFX10 sfx; | ||
| sfx.guid = "effects.Warping"; | ||
| sfx.entityID = mySE->GetID(); | ||
| sfx.isOffensive = false; | ||
| sfx.start = false; | ||
| sfx.active = false; | ||
| if (m_ballMode == Destiny::Ball::Mode::WARP) { | ||
| sfx.start = true; // TODO: verify if this is necessary | ||
| sfx.active = true; | ||
| } | ||
|
|
||
| updates.push_back(sfx.Encode()); | ||
| SendDestinyUpdate(updates); | ||
| updates.clear(); | ||
|
|
||
| SetState ss; | ||
| ss.stamp = sEntityList.GetStamp(); | ||
| ss.ego = mySE->GetID(); | ||
|
|
||
| ss.stamp = sEntityList.GetStamp(); | ||
| ss.ego = mySE->GetID(); | ||
|
|
||
| if (mySE->SysBubble() == nullptr) { | ||
| // returning here preemptively avoids a segfault (good), but isn't a | ||
| // good situation to encounter. | ||
| sLog.Error( | ||
| "DestinyManager::SendSetState()", | ||
| "Destiny::SendSetState() the player isn't in a system bubble! Aborting attempt to send state." | ||
| ); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Updating the sendstate function here for two reasons:
- (unsuccessfully) attempting to fix the 'warp drive active' client/server desync issue that sometimes happens. This issue really bothers me but I still haven't figured it out.
- gracefully avoiding the segfault that occurs when attempting to sendstate while having a
nullptrsysbubble.
| if (is_log_enabled(DESTINY__UPDATES)) { | ||
| _log( | ||
| DESTINY__UPDATES, | ||
| "[%u] BubbleCasting global structure destiny update (u:%u, e:%u) for stamp %u to all bubbles from %s(%u)", | ||
| sEntityList.GetStamp(), | ||
| updates.size(), | ||
| events.size(), | ||
| sEntityList.GetStamp(), | ||
| (mySE->HasPilot()?mySE->GetPilot()->GetName():mySE->GetName()), | ||
| (mySE->HasPilot()?mySE->GetPilot()->GetCharID():mySE->GetID()) | ||
| ); | ||
| } | ||
|
|
||
| //Get all clients in the system which the SE is in | ||
| /** @todo this isnt right....will segfault. needs to be fixed */ | ||
| std::vector<Client*> cv; | ||
|
|
||
| mySE->SystemMgr()->GetClientList(cv); | ||
|
|
There was a problem hiding this comment.
more simple code formatting fixes
| DESTINY__ERROR, | ||
| "[%u] Cannot BubbleCast destiny update (u:%u, e:%u); entity (%u) is not in any bubble.", | ||
| sEntityList.GetStamp(), | ||
| updates.size(), | ||
| events.size(), | ||
| mySE->GetID() | ||
| ); | ||
|
|
||
| // if (sConfig.debug.IsTestServer) { | ||
| // EvE::traceStack(); | ||
| // } | ||
|
|
There was a problem hiding this comment.
more simple code formatting fixes, but also removes the traceStack call because it isn't really necessary for this situation (based on my experience debugging destiny).
| @@ -296,7 +296,7 @@ class DestinyManager { | |||
| float m_maxOrbitSpeedFraction; //fuzzy logic - ship's max speed based on orbit data | |||
|
|
|||
| uint32 m_followDistance; //in m | |||
| uint32 m_targetDistance; //in m | |||
| double m_targetDistance; //in m | |||
| @@ -193,62 +193,106 @@ void SystemBubble::ProcessWander(std::vector<SystemEntity*> &wanderers) { | |||
| ResetBubbleRatSpawn(); | |||
| } | |||
|
|
|||
| void SystemBubble::Add(SystemEntity* pSE) | |||
| { | |||
| void SystemBubble::Add(SystemEntity* pSE) { | |||
There was a problem hiding this comment.
everything in this file was just code formatting fixes; no actual changes intended
| @@ -216,7 +216,7 @@ class SystemEntity { | |||
| virtual bool IsDungeonEditSE() { return false; } | |||
|
|
|||
| /* generic functions handled here */ | |||
| EVEServiceManager& GetServices() { return m_services; } | |||
| EVEServiceManager& GetServices() { return m_services; } | |||
Reviewer's Guide by SourceryThis pull request changes the data type of Class diagram for DestinyManager changesclassDiagram
class DestinyManager {
+double m_targetDistance
+void InitWarp()
+void WarpAccel(uint16 sec_into_warp)
+void WarpCruise(uint16 sec_into_warp)
+void WarpDecel(uint16 sec_into_warp)
+void WarpUpdate(double currentShipSpeed)
+void WarpStop(double currentShipSpeed)
+void BeginMovement()
+void WarpTo(GPoint where, int32 distance, bool autoPilot)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @charles-m-knox - I've reviewed your changes - here's some feedback:
Overall Comments:
- The change from
uint32todoubleform_targetDistanceis a good fix for the overflow issue. Consider reviewing other distance-related variables for similar issues. - The
Halt()call after warp stop seems like a bandaid fix. Please investigate the root cause of the ball mode issue instead of relying on this workaround, as it may introduce client-server desynchronization. - Consider consolidating some of the logging statements to reduce code duplication, particularly in the
WarpToandAddmethods.
Here's what I looked at during the review
- 🟡 General issues: 3 issues 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 and I'll use the feedback to improve your reviews.
|
|
||
| double warpSpeedInMeters(m_shipWarpSpeed * ONE_AU_IN_METERS); | ||
| double warpSpeedInMeters(static_cast<double>(m_shipWarpSpeed) * static_cast<double>(ONE_AU_IN_METERS)); |
There was a problem hiding this comment.
suggestion: Consider using consistent floating-point types
While the use of static_cast is good for explicit conversions, consider standardizing on either float or double for all space-related calculations throughout the codebase. This would improve consistency and prevent potential precision loss from type conversions.
| double warpSpeedInMeters(static_cast<double>(m_shipWarpSpeed) * static_cast<double>(ONE_AU_IN_METERS)); | |
| double warpSpeedInMeters = m_shipWarpSpeed * ONE_AU_IN_METERS; |
| _log( | ||
| DESTINY__WARP_TRACE, | ||
| "Destiny::InitWarp(): %s(%u) has initialized warp.", | ||
| mySE->GetName(), | ||
| mySE->GetID() | ||
| ); |
There was a problem hiding this comment.
suggestion (performance): Consider adding log levels or conditional compilation for verbose logging
While detailed logging is helpful for debugging, consider implementing log levels or conditional compilation directives to reduce the performance impact and log verbosity in production environments.
#ifdef ENABLE_VERBOSE_LOGGING
_log(
DESTINY__WARP_TRACE,
"Destiny::InitWarp(): %s(%u) has initialized warp.",
mySE->GetName(),
mySE->GetID()
);
#endif
| // TODO: when exiting warp, and attempting to warp again shortly after, the | ||
| // ball mode reaches a weird state where it goes from Warp to a regular | ||
| // move. Halting the ship after warp completes seems to fix this, but it's | ||
| // not a good fix, because the client shows that the ship moves a few meters | ||
| // forward while decelerating - meaning that the client and server are | ||
| // briefly out of sync because the server thinks the ship is halted. | ||
| Halt(); |
There was a problem hiding this comment.
suggestion (bug_risk): Investigate root cause of warp exit issue instead of using Halt()
Rather than using Halt() as a workaround, it would be better to investigate and address the root cause of the warp exit issue. This temporary fix may introduce other synchronization problems between the client and server.
| // TODO: when exiting warp, and attempting to warp again shortly after, the | |
| // ball mode reaches a weird state where it goes from Warp to a regular | |
| // move. Halting the ship after warp completes seems to fix this, but it's | |
| // not a good fix, because the client shows that the ship moves a few meters | |
| // forward while decelerating - meaning that the client and server are | |
| // briefly out of sync because the server thinks the ship is halted. | |
| Halt(); | |
| // Investigate root cause of warp exit issue | |
| // Potential areas to check: | |
| // 1. State transition logic in ball mode | |
| // 2. Timing of warp completion events | |
| // 3. Synchronization between client and server movement | |
| // 4. Cooldown period after warp before allowing new actions | |
| // Temporary logging for debugging | |
| sLog.Debug("DestinyManager", "Warp exit at position: %f, %f, %f", GetPosition().x, GetPosition().y, GetPosition().z); |
|
Yep this all looks good to me, I would definitely be interested to see what other similar issues are lurking in the code, being the root cause of a lot of desync to the client. Lets get this merged and see where we stand :) |
Thanks @jdhirst , I'll keep at it. Would love to see this stabilize. |
If you enable a notification that shows when the server thinks you've stopped warping, you'll see that it happens very quickly after starting warp. I suspect this is because destiny's
m_targetDistance(used in warp calculations) is currently auint32value, which cannot exceed 4 billion meters. 1 AU is 150 billion meters, so I suspect the data type is overflowing and we're seeing odd behavior with destiny that causes the server to think warp is finishing way too early.This PR changes the data type from
uint32todouble. It seems like this stabilizes the calculations quite a bit, but it still isn't perfect.The other significant change in this PR is that during warp cruise, a sysbubble is allowed to be created on every server tick. This allows the server to correctly act on its assumptions that the player is always in a sysbubble when in space. Previously it was not in a sysbubble during warp cruise. A side effect of this is that the player is repeatedly entering and exiting sysbubbles during every second of warp, but I don't see any other way to implement it currently.
Also included in this PR are some other changes:
Halt()after warp stop occurs, for slightly better stability (this requires further investigation)WARPtoGOTOafter the next tick (instead of staying onWARPfor as many ticks as it takes to finish the whole warp). Halting after warp seems to prevent this. This is a bandaid fix, because without it, warping anywhere breaks in a very annoying way (the server just thinks you're moving in the direction of warp forever and it never actually warps you, but the client does).abs()for distance calculation when determining if the warp distance is less than the warp speed of the ship/setstate(destiny sendstate call specifically) early if the player is in anullptrsysbubble to avoid a segfaultI'm leaving this pull request in draft state for now so that I can test it out for a little while. I don't anticipate the code changing too much more though, so feel free to add any review comments/thoughts.
Summary by Sourcery
Change m_targetDistance from uint32 to double to fix warp calculation overflow issues, enhance warp stability with additional logic, and refactor code for improved precision and readability.
Bug Fixes:
Enhancements: