-
Notifications
You must be signed in to change notification settings - Fork 122
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
Asteroids break into smaller pieces and then into tiny rocks. Small pieces drop loot. #654
base: develop
Are you sure you want to change the base?
Conversation
8f0cbdf
to
52d2b8c
Compare
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.
I haven't tested this yet but a few things caught my eye whilst looking through the changes.
@@ -146,6 +147,12 @@ private void buildRubblePieces(Position pos, Velocity vel, Angle angle, Size siz | |||
|
|||
EntityRef entityRef = entitySystemManager.getEntityManager().createEntity(graphicsComponent, positionComponent, | |||
velocityComponent, angle, sizeComponent, new RubbleMesh()); | |||
|
|||
if(sizeComponent.size > 0.1) { |
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.
You might want to move the 0.1
value into a MIN_DIVISIBLE_SIZE
constant. I wasn't clear on why that value was chosen until I realised what it was.
@@ -136,7 +137,7 @@ private void buildRubblePieces(Position pos, Velocity vel, Angle angle, Size siz | |||
|
|||
//Create size component | |||
Size sizeComponent = new Size(); | |||
sizeComponent.size = scale; | |||
sizeComponent.size = scale * size.size; |
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.
GitHub doesn't let me comment on unchanged lines but you forgot to multiply by the parent size on line 129.
DestinationSol/engine/src/main/java/org/destinationsol/rubble/systems/RubbleCreationSystem.java
Lines 128 to 130 in 52d2b8c
Vector2 position = new Vector2(); | |
SolMath.fromAl(position, velocityAngle, SolRandom.randomFloat(size.size)); | |
position.add(basePos); |
Why not create a local variable to store the scaled size?
float scaledSize = scale * size.size;
You can then use that variable everywhere where you are currently using scale * size.size
.
@BenjaminAmos Thnkx for the review. I have made the corrections. |
…d then into tiny rocks. The smaller pieces also drop loot when destroyed.
a27f01d
to
58cbfde
Compare
Rebased to develop |
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.
The fix works and the changes seem reasonable to me. I've never seen rubble as large as this before! I'll approve this but will wait for another review if possible, since I'm not too familiar with this area of the code,
Description
Testing
P.S: The newly created pieces of asteroid do not break(fixed in #653). To test, add the health component to the rubble entities.
Pre Pull Request Checklist: