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.

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

Water/Thirst system improvements and modifications #69

Merged
merged 14 commits into from
Jul 23, 2020

Conversation

AndyTechGuy
Copy link
Collaborator

@AndyTechGuy AndyTechGuy commented Jun 15, 2020

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

  • Wells can now be drunk from directly. This replenishes all of a player's thirst.
  • Instead of being given a water cup when interacting with a well, empty water cups can be bought from the market, and filled with a well.
  • Wells have a capacity of drinks, and the player will need to wait for a bit to drink again from this well when this limit is reached.
  • Tooltips have been included to indicate the status of wells.

image
A full well, and an empty well.

Testing

  • Travel out in the world until a city spawns. Find a well (3 by 3 structure with water in the middle)
  • Look at the water and press 'e' with nothing in your hand. Your thirst (the blue bar above your hotbar) should replenish instantly.
  • Wait a little longer until a marketplace spawns, and press 'e' on the white gooey character walking around. Buy the empty cup from the shop.
  • Return to the well and press 'e' while looking at the water. The cup should fill with water. Drink it by right clicking, and it should become empty again.
  • Interact with the well by pressing 'e' until the tooltip reads "Drinks: 0/5". After this point, no thirst should be refilled by interacting with it again.
  • Wait for a while when a well isn't at full capacity. After a bit of time, the well should replenish, one drink at a time.

@AndyTechGuy AndyTechGuy marked this pull request as ready for review July 4, 2020 00:57
@skaldarnar
Copy link
Contributor

I'll have a look at this later, but I'm already wondering whether we can split this up into smaller parts (for review)?

assets/prefabs/items/waterCup.prefab Outdated Show resolved Hide resolved
"name": "Empty Cup"
},
"Item": {
"stackId": "MetalRenegades:emptyCup",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines 1 to 15
/*
* 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.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

We are using a shorter license header format now (should be automatically used with 2020.x IntelliJ when using the pure Gradle setup).

Suggested change
/*
* 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

Copy link
Collaborator Author

@AndyTechGuy AndyTechGuy Jul 6, 2020

Choose a reason for hiding this comment

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

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.

}

@ReceiveEvent(components = {WellBlockComponent.class})
public void onActivate(ActivateEvent event, EntityRef sourceBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines 145 to 157
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

With a more functional approach to using Stream API this could be written as

Suggested change
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);
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

... or with using a Predicate<EntityRef>:

Suggested change
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);
        };
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 skaldarnar added the Type: Improvement Request for or addition/enhancement of a feature label Jul 4, 2020
@AndyTechGuy AndyTechGuy merged commit add6d51 into Terasology:develop Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement Request for or addition/enhancement of a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants