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.

Sign up for GitHub

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

Jump to bottom

fix: NPE in EconomySystem #26

Merged
merged 7 commits into from
Nov 1, 2020
Merged

fix: NPE in EconomySystem #26

merged 7 commits into from
Nov 1, 2020

Conversation

jdrueckert
Copy link
Contributor

@jdrueckert jdrueckert commented Oct 27, 2020 •
edited
Loading

How to test

  1. Start a GooKeeper game without this PR
  2. Spawn a visitor with spawnPrefab visitor
  3. Observe that game crashing
  4. Repeat (1) and (2) with this PR and observe that the game doesn't crash and the visitor is spawned correctly (visitor will likely be invisible without changes from fix(gltf): replace deprecated md5 assets #27 being present).

@jdrueckert jdrueckert added the Type: Bug Issues reporting and PRs fixing problems label Oct 27, 2020
@jdrueckert jdrueckert self-assigned this Oct 27, 2020
@jdrueckert jdrueckert mentioned this pull request Oct 27, 2020
Merged
skaldarnar
skaldarnar previously approved these changes Oct 28, 2020
View reviewed changes
Copy link
Member

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that the game no longer crashes with this PR. I wasn't able to see the visitor gooey, though (probably fixed by #27 ?)

src/main/java/org/terasology/gookeeper/system/EconomySystem.java
economyComponent.playerWalletCredit += baseEntranceFee;
player.saveComponent(economyComponent);
public void payEntranceFee(VisitorComponent visitorComponent) {
if (visitorComponent.visitorEntranceBlock.hasComponent(VisitorEntranceComponent.class)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log something in case the component is not there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, I'll take care of it and re-request review afterwards 👍

@jdrueckert @skaldarnar
Update src/main/java/org/terasology/gookeeper/system/EconomySystem.java
a36ed15 
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
@jdrueckert jdrueckert requested a review from skaldarnar October 28, 2020 18:43
skaldarnar
skaldarnar reviewed Oct 31, 2020
View reviewed changes
src/main/java/org/terasology/gookeeper/system/EconomySystem.java Outdated

VisitBlockComponent visitBlockComponent = visitBlock.getComponent(VisitBlockComponent.class);

Prefab gooeyPrefab = prefabManager.getPrefab("GooKeeper:" + visitBlockComponent.type);

if (economyComponent != null && gooeyPrefab != null && gooeyPrefab.hasComponent(GooeyComponent.class)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the checks for gooeyPrefab != null and gooeyPrefab.hasComponent(GooeyComponent.class)?

This might still be breaking - is this such that it fails hard if these dependencies within the same module break?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch... that wasn't on purpose tbh... and yes I think if the getComponent returns null and that is left unchecked, the resulting NPE would crash the game hard.

@skaldarnar
Copy link
Member

That's quite a huge visitor 🙄

Terasology-201031150215-1280x800

@jdrueckert
Copy link
Contributor Author

That's quite a huge visitor roll_eyes

Terasology-201031150215-1280x800

I believe that's fixed by #27

@skaldarnar
Copy link
Member

Spawning a visitor now yields a lot of log spam 🙈

19:41:35.419 [main] WARN  o.t.gookeeper.system.EconomySystem - VisitorComponent.visitorEntranceBlock requires VisitorEntranceComponent for payEntranceFee

I think this warning is added in this PR. Was our assumption correct that this component should always be there? Is it added if the visitor is spawned "by the game"? Should we remove the warning again? And why is that checked so often... so many questions 🙈

@jdrueckert
Copy link
Contributor Author

Was our assumption correct that this component should always be there? Is it added if the visitor is spawned "by the game"?

payEntranceFee is called on spawning a visitor. Without the visitorEntranceBlock, this method will have no effect which I don't think is intended. The issue IMO is with the debug spawnPrefab command. To my knowledge usually in GooKeeper you place an entrance block for your zoo which will spawn visitors that then of course hav that visitorEntranceBlock.

Should we remove the warning again?

Don't think so. We should consider how to make it clearer that it's expected when spawning using the debugging command spawnPrefab...

And why is that checked so often...

Good question, I don't think it should be checked that often 🤔

@jdrueckert
Copy link
Contributor Author

Fixed the gigantic visitor model in #29

@skaldarnar skaldarnar merged commit 0810c2e into develop Nov 1, 2020
@skaldarnar skaldarnar deleted the fix/economysystem-NPE branch November 1, 2020 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skaldarnar skaldarnar skaldarnar approved these changes

Assignees

@jdrueckert jdrueckert

Labels
Type: Bug Issues reporting and PRs fixing problems
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdrueckert @skaldarnar