Water/Thirst system improvements and modifications #69
Conversation
|
I'll have a look at this later, but I'm already wondering whether we can split this up into smaller parts (for review)? |
src/main/java/org/terasology/metalrenegades/interaction/component/WellSourceComponent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/terasology/metalrenegades/interaction/component/WellSourceComponent.java
Outdated
Show resolved
Hide resolved
| "name": "Empty Cup" | ||
| }, | ||
| "Item": { | ||
| "stackId": "MetalRenegades:emptyCup", |
There was a problem hiding this comment.
I don't like that we copy-paste the cup icon around - we should find a way to share this between JS (ManualLabor?) and MetalRenegades. Please have a look at the discussion in Terasology/ManualLabor#31
There was a problem hiding this comment.
Yeah it would be very nice to be able to take some of the more useful parts of ManualLabor without needing to use the more complicated portions of that module. If the fluid containers portion of ML could be separated into it's own module that would be fantastic.
If I remember right I think that a lot of the texture assets in ManualLabor come from a Spritesheet (I had to crop it out of the sheet myself). I don't know how easy that would be to separate into different modules.
| /* | ||
| * Copyright 2020 MovingBlocks | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ |
There was a problem hiding this comment.
We are using a shorter license header format now (should be automatically used with 2020.x IntelliJ when using the pure Gradle setup).
| /* | |
| * Copyright 2020 MovingBlocks | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * You may obtain a copy of the License at | |
| * | |
| * http://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| */ | |
| // Copyright 2020 The Terasology Foundation | |
| // SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
I applied the headings change to this PRs files. I can't seem to get the new header to automatically apply to new files though, and I don't know where I would look to find this option.... I'm using IntelliJ 2020.1.2 Ultimate.
src/main/java/org/terasology/metalrenegades/interaction/systems/WellWaterSystem.java
Outdated
Show resolved
Hide resolved
src/main/java/org/terasology/metalrenegades/interaction/systems/WellWaterSystem.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @ReceiveEvent(components = {WellBlockComponent.class}) | ||
| public void onActivate(ActivateEvent event, EntityRef sourceBlock) { |
There was a problem hiding this comment.
I don't like that there are so many return statements in this method. However, the nesting is not very deep, so it's still well comprehensible...
There was a problem hiding this comment.
I did some rearranging of this method to make it look a bit cleaner, and removed a few of the return statements. There's still two of them though... Too many return statements is definitely something I'll be looking out for in the future.
src/main/java/org/terasology/metalrenegades/interaction/systems/WellWaterSystem.java
Outdated
Show resolved
Hide resolved
| EntityRef wellEntity = null; | ||
|
|
||
| for (EntityRef waterSource : entityManager.getEntitiesWith(WellSourceComponent.class)) { | ||
| DynParcelRefComponent dynParcelRefComponent = waterSource.getComponent(DynParcelRefComponent.class); | ||
| Rect2i parcelRect = dynParcelRefComponent.dynParcel.getShape(); | ||
|
|
||
| if (parcelRect.contains(blockLocation.x, blockLocation.z)) { | ||
| wellEntity = waterSource; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return wellEntity; |
There was a problem hiding this comment.
With a more functional approach to using Stream API this could be written as
| EntityRef wellEntity = null; | |
| for (EntityRef waterSource : entityManager.getEntitiesWith(WellSourceComponent.class)) { | |
| DynParcelRefComponent dynParcelRefComponent = waterSource.getComponent(DynParcelRefComponent.class); | |
| Rect2i parcelRect = dynParcelRefComponent.dynParcel.getShape(); | |
| if (parcelRect.contains(blockLocation.x, blockLocation.z)) { | |
| wellEntity = waterSource; | |
| break; | |
| } | |
| } | |
| return wellEntity; | |
| return StreamSupport.stream(entityManager.getEntitiesWith(WellSourceComponent.class).spliterator(), false) | |
| .filter(waterSource -> containsLocation(waterSource, blockLocation)) | |
| .findFirst() | |
| .orElse(EntityRef.NULL); |
The method containsLocation probably needs a better name, but I don't fully get what is happening there to be honest 🙈
private boolean containsLocation(EntityRef waterSource, Vector3i location) {
DynParcelRefComponent dynParcelRefComponent = waterSource.getComponent(DynParcelRefComponent.class);
Rect2i parcelRect = dynParcelRefComponent.dynParcel.getShape();
return parcelRect.contains(location.x, location.z);
}There was a problem hiding this comment.
... or with using a Predicate<EntityRef>:
| EntityRef wellEntity = null; | |
| for (EntityRef waterSource : entityManager.getEntitiesWith(WellSourceComponent.class)) { | |
| DynParcelRefComponent dynParcelRefComponent = waterSource.getComponent(DynParcelRefComponent.class); | |
| Rect2i parcelRect = dynParcelRefComponent.dynParcel.getShape(); | |
| if (parcelRect.contains(blockLocation.x, blockLocation.z)) { | |
| wellEntity = waterSource; | |
| break; | |
| } | |
| } | |
| return wellEntity; | |
| return StreamSupport.stream(entityManager.getEntitiesWith(WellSourceComponent.class).spliterator(), false) | |
| .filter(atLocation(blockLocation)) | |
| .findFirst() | |
| .orElse(EntityRef.NULL); |
and
private Predicate<EntityRef> atLocation(Vector3i location) {
return (waterSource) -> {
DynParcelRefComponent dynParcelRefComponent = waterSource.getComponent(DynParcelRefComponent.class);
Rect2i parcelRect = dynParcelRefComponent.dynParcel.getShape();
return parcelRect.contains(location.x, location.z);
};
}There was a problem hiding this comment.
Woah, the stream implementation looks really nice! I'm still not very well versed in using streams over ordinary loops, I'll need to do some research on that. 👍
skaldarnar
added
the
Type: Improvement
src/main/java/org/terasology/metalrenegades/interaction/component/WellSourceComponent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/terasology/metalrenegades/interaction/systems/WellWaterSystem.java
Outdated
Show resolved
Hide resolved
This pull request includes some major changes to the way that the thirst need is restored by players. These should make it so the thirst need is a greater challenge for players to fulfill.
All Changes
A full well, and an empty well.
Testing