[#2438, #2377, #2435] Support RockSim booster importing/exporting, pods/booster splitting & fix a bunch of bugs #2447
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change is to ensure that the stageMap property of the Rocket class is isolated from its source object. Previously, it directly referenced the stageMap of the source, but now we create an entirely new ConcurrentHashMap to hold those stages. This will prevent possible conflicts and unwanted modifications to the source stageMap.
This should not only improve efficiency, but also help with unwanted side effects of the component still having listeners even when the config dialog is disposed.
…lidate This caused issue because the invalidating of invalidatables was sometimes triggered by AWT processes
This fixes an issue where undoing/redoing stage activeness actions wouldn't work.
SiboVG
changed the title
Contributor
|
Wow, quite an epic PR. Nice work. Can't wait to try to summarize that in the release notes. :) |
Contributor
|
Wow, this is an amazing piece of work! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Okay, what started out as a small PR to fix #2438 quickly turned into much more... I'll take you through my rollercoaster.
The issue with #2438 was that exporting booster designs to RockSim would throw an error because booster exporting simply wasn't implemented. So, I started implementing it.
Then I noticed that OR only exported one instance of the pods/boosters. Also, the rotation of children components of the pod set was incorrect. That needed fixing. To do that, I needed to split a pod set/booster into separate pod set/booster instances and then export each instance to RockSim. Hey, would you look at that! I just fixed #2377. You can now split pods and boosters:
Okay, cool, worked great for pods, but crashed for boosters... After a bunch of investigating, I found something interesting. DoubleModels (or any other model) in component config dialogs weren't properly invalidated when the config dialog closed. This caused those models to remain listeners of the corresponding rocket component, thus activating whenever the component changed state. Not only did that cause my bug (and possible future bugs), it is also inefficient - the models also weren't garbage collected. So, I rewrote DoubleModel and other models to support proper invalidation, and then registered every model in each config dialog so that they would invalidate when the config dialog closed. Okay, great, so that bug's now fixed + we have a slight optimization of OpenRocket (not that I could notice it in OR's RAM usage, but hey, it's something).
Then, while playing around with the booster split functionality, I found a way to reliably trigger bug #2435. I was able to trigger it by changing the rocket's stageMap configuration (by splitting the boosters) and then undoing and redoing that operation 2 times. This PR now also fixes that bug. The issue was that
loadFromin the Rocket did not copy the stageMap of the source rocket, but instead referenced it. In some scenarios the source rocket's stage map would change, which also caused the stage map in the target rocket to change. This PR now ensures that the stage map is correctly copied.With the pod/booster split functionality now working, I was able to code the RockSim booster exporting:
Then I thought "Hey, I should also support booster importing from a RockSim design" (a booster in RockSim is a pod that can be ejected). So I did that.
Almost done, I swear.
Finally, when playing around with the booster split functionality, I noticed that undoing/redoing changes to stage activeness did not work. If I disabled a booster stage, and then undo that operation, then the booster stage was enabled again - as expected. However, when redoing the operation, the stage activeness did not change. The issue was that stage activeness was not copied when copying/loading a rocket. This PR fixes that.
In summary: