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

Implement a functioning Blood Tank #969

Merged
merged 16 commits into from
Dec 12, 2016
Merged

Implement a functioning Blood Tank #969

merged 16 commits into from
Dec 12, 2016

Conversation

Arcaratus
Copy link
Contributor

@Arcaratus Arcaratus commented Nov 8, 2016

-Added a search bar to the Upgrade Tomes Creative Tab
-Updated some Altar fluid code (remove deprecated stuff)
-Moved Rendering classes into appropriate package
-Fix the localization errors on the Demon Crystals
-A few cleanups here and there
-Removed obsolete fluid methods in Utils
-Has comparator output
-Update JEI compat

-- TO DO --
-Add recipes for the Blood Tank
-Fix the lighting-not-updating-upon-fluid-placement within the tank bug
-Render tank in inventory if possible <- Terrible but working
-Future (maybe 1.11) update from deprecated methods and classes throughout entire mod
-Fix JEI bug where cheating a tank in reverts to a Tier 1 tank regardless of which tank you cheated in

Added a search bar to the Upgrade Tomes Creative Tab
Updated some Altar fluid code (remove deprecated stuff)
Moved Rendering classes into appropriate package
Fix the localization errors on the Demon Crystals
A few cleanups here and there
@TehNut
Copy link
Collaborator

TehNut commented Nov 8, 2016

Again, please no TESR. Use the model system.

I will look over this more when I get home

if (!config.getConfig(Constants.Compat.WAILA_CONFIG_BLOOD_TANK))
return currenttip;

if (accessor.getPlayer().isSneaking() || config.getConfig(Constants.Compat.WAILA_CONFIG_BYPASS_SNEAK))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole bit is wrong. You cannot access a Tiles NBT here. Write everything you need to the tag in #getNBTData and read it from accessor#getNBTData. I need to fix our other implementations to do this. See the altar impl for an example.

@@ -17,108 +25,144 @@
public ItemBlockBloodTank(Block block)
{
super(block);

setHasSubtypes(true);
setMaxDamage(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unecessary.

tile.BloodMagic.demonCrystal.corrosive.name=Corrosive Will Crystal Cluster
tile.BloodMagic.demonCrystal.destructive.name=Destructive Will Crystal Cluster
tile.BloodMagic.demonCrystal.vengeful.name=Vengeful Will Crystal Cluster
tile.BloodMagic.demonCrystal.steadfast.name=Steadfast Will Crystal Cluster

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you change this here, you need to change every language.

@WayofTime
Copy link
Owner

Good to pull, or need more stuff?

@TehNut
Copy link
Collaborator

TehNut commented Nov 9, 2016

Needs tank rewritten as a model instead of a TESR. If this is being done, it should be done correctly.

@WayofTime
Copy link
Owner

@TehNut Talked to both Grum and cpw. They said that a TESR for any form of block that updates its model regularly is a bad idea because it redraws the chunk of 16^3 blocks that it is in. So the static part of the model (the block) should be a model, but the fluid part should either be a regular TESR or a fast TESR.

@TehNut
Copy link
Collaborator

TehNut commented Nov 10, 2016

Alright, let's do that, then.

"tier": {
"0": {
"textures": {
"all": "bloodmagic:blocks/BloodTank"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you add

"textures": {
    "all": "bloodmagic:blocks/BloodTank"
}

to the "default": {} block, you shouldn't need it in all of these variants.

currenttip.add(TextHelper.localizeEffect("tooltip.BloodMagic.fluid.capacity") + ": " + bloodTank.getCapacity() + "mB");

NBTTagCompound tag = accessor.getNBTData().getCompoundTag(Constants.NBT.TANK);
if (!Strings.isNullOrEmpty(tag.getString("FluidName")) && FluidStack.loadFluidStackFromNBT(tag) != null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Save the FluidStack instead of creating it from NBT twice.

if (!Strings.isNullOrEmpty(tag.getString("FluidName")) && FluidStack.loadFluidStackFromNBT(tag) != null)
{
currenttip.add(TextHelper.localizeEffect("tooltip.BloodMagic.fluid.type") + ": " + FluidStack.loadFluidStackFromNBT(tag).getLocalizedName());
currenttip.add(TextHelper.localizeEffect("tooltip.BloodMagic.fluid.amount") + ": " + tag.getInteger("Amount") + "/" + bloodTank.getCapacity() + "mB");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This getCapacity() call (and the one above) need to either be changed to be retrieved from NBT or the getCapacity() method needs to be changed to not depend on a field obtained via NBT.

tessellator.draw();
}

public static TextureAtlasSprite checkIcon(TextureAtlasSprite icon)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why create this method? You run this bit of code once, as far as I can tell.

@TehNut
Copy link
Collaborator

TehNut commented Nov 11, 2016

Whitespacing seems to be a mix of tabs and spaces. Please convert all of the tabs to spaces.

@WayofTime
Copy link
Owner

That said, I'll probably be running Eclipse's formatter afterwards, so the last part isn't as bad of an issue.

@@ -99,4 +99,9 @@ public void register(@Nonnull IModRegistry registry)
jeiHelper.getNbtIgnoreList().ignoreNbtTagNames(Constants.NBT.DIMENSION_ID);
jeiHelper.getNbtIgnoreList().ignoreNbtTagNames(Constants.NBT.ITEM_INVENTORY);
}

@Override
public void onRuntimeAvailable(IJeiRuntime runtime)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why implement this? BlankModPlugin should be taking care of any unneeded methods.


/**
* Shamelessly taken off of the Mekanism repository written by aidancbrady
* https://github.com/aidancbrady/Mekanism
Copy link
Collaborator

Choose a reason for hiding this comment

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

@author aidancbrady

@TehNut TehNut merged commit aac2623 into WayofTime:1.9 Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants