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

Finished Fluid Integration #3510

Merged
merged 20 commits into from Jun 11, 2018

Conversation

Projects
None yet
4 participants
@BrockWS
Contributor

BrockWS commented May 24, 2018

Finishes #3341

Adds Fluid Import/Export Bus
Adds Fluid Storage Bus
Adds Fluid Terminal

All Fluid Buses work like normal Item Buses, just use buckets to set the filters.

Fluid Terminal currently works as follows:

  • Left clicking while holding a bucket or tank to fill it up, unless its already full.
  • Right clicking while holding a bucket or tank to drain it.

This PR does NOT include custom textures.

@yueh

This comment has been minimized.

Member

yueh commented May 24, 2018

I probably need a while to review it. Also most likely in multiple passes. At least I will try to give some initally feedback as fast as possible

@BrockWS BrockWS referenced this pull request May 24, 2018

Closed

Deprecate Fluid Cells and Fluid Buses #566

5 of 5 tasks complete
@BrockWS

This comment has been minimized.

Contributor

BrockWS commented May 24, 2018

Understood, if anything is wrong i'll work it out ASAP

@yueh

Also consider naming conventions for textures/models/translations.

IItemDefinition fluidCell64kPart();
IItemDefinition emptyFluidStorageCell();

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

Is there a reason for it? Could just reuse the normal empty storage housing just in case someone wants to convert them back and forth.

This comment has been minimized.

@BrockWS

BrockWS May 24, 2018

Contributor

Drummer implemented it this way, but I agree, having just 1 storage housing is better. Should be an easy change as I just realized the recipes were not done

IItemDefinition fluidTerminal();
IItemDefinition importBusFluids();

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

Still not sure about the naming conventions. Currently it is not really consistent with sometimes using plural, sometimes singular. Also prefix, infix, and postfix is randomly used. Mostly to avoid something like the API name is importBusFluids, the class uses ImportFluidBus and the texture import_bus_fluid.png and the model fluid_import_bus. Just not sure about the best one currently, but at first glance something lile fluidImportBus makes most sense, also when looking at the terminal with fluidTerminal

IItemDefinition importBusFluids();
IItemDefinition exportBusFluids();

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

Naming

IItemDefinition exportBusFluids();
IItemDefinition storageBusFluids();

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

Naming

IItemDefinition storageBusFluids();
IItemDefinition levelEmitterFluids();

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

Naming

@@ -109,8 +112,7 @@ public ApiParts( FeatureFactory registry, PartModels partModels )
this.iface = new DamagedItemDefinition( "part.interface", itemPart.createPart( PartType.INTERFACE ) );
this.levelEmitter = new DamagedItemDefinition( "part.level_emitter", itemPart.createPart( PartType.LEVEL_EMITTER ) );
this.annihilationPlane = new DamagedItemDefinition( "part.plane.annihilation", itemPart.createPart( PartType.ANNIHILATION_PLANE ) );
this.identityAnnihilationPlane = new DamagedItemDefinition( "part.plane.annihiliation.identity", itemPart
.createPart( PartType.IDENTITY_ANNIHILATION_PLANE ) );
this.identityAnnihilationPlane = new DamagedItemDefinition( "part.plane.annihiliation.identity", itemPart.createPart( PartType.IDENTITY_ANNIHILATION_PLANE ) );

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

Formatting

* @version rv6 - 30/04/2018
* @since rv6 30/04/2018
*/
public class PartImportBusFluid extends PartSharedFluidBus

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

Naming

* @version rv6 - 22/05/2018
* @since rv6 22/05/2018
*/
public class PartLevelEmitterFluid extends PartLevelEmitter

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

Naming

// *******************************************
// Class Methods
// *******************************************

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

The IDE should take care of highlighting/grouping interface methods, not some random and outdated comments.

* @version rv6 - 22/05/2018
* @since rv6 22/05/2018
*/
public class FluidSorters

This comment has been minimized.

@yueh

yueh May 24, 2018

Member

This should probably take #3434 into account

Naming and formatting changes.
Removed the fluid storage housing.
Implemented #3434 for FluidSorters
@BrockWS

This comment has been minimized.

Contributor

BrockWS commented May 27, 2018

I still need to add fluid cell recipes, thinking of using the same as EC2 (4 Lapis, 4 Quartz, Logic Processor)

final int xo = 8;
final int yo = 23 + 6;
final IItemHandler config = this.getUpgradeable().getInventoryByName( "config" );

This comment has been minimized.

@orod-org

orod-org May 27, 2018

CRITICAL Define a constant instead of duplicating this literal "config" 3 times. rule

this.sortSrc = sortSrc;
}
public void updateView()

This comment has been minimized.

@orod-org

orod-org May 27, 2018

CRITICAL The Cyclomatic Complexity of this method "updateView" is 20 which is greater than 10 authorized. rule

for( final String line : tooltip )
{
if( m.matcher( line ).find() )

This comment has been minimized.

@orod-org

orod-org May 27, 2018

CRITICAL Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule

@@ -129,6 +129,9 @@
ReportInaccessibleItems,
ReportInaccessibleItemsYes,
ReportInaccessibleItemsNo,
ReportInaccessibleFluids,

This comment has been minimized.

@orod-org

orod-org May 27, 2018

CRITICAL Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'. rule

@@ -129,6 +129,9 @@
ReportInaccessibleItems,
ReportInaccessibleItemsYes,
ReportInaccessibleItemsNo,
ReportInaccessibleFluids,
ReportInaccessibleFluidsYes,

This comment has been minimized.

@orod-org

orod-org May 27, 2018

CRITICAL Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'. rule

@@ -129,6 +129,9 @@
ReportInaccessibleItems,
ReportInaccessibleItemsYes,
ReportInaccessibleItemsNo,
ReportInaccessibleFluids,
ReportInaccessibleFluidsYes,
ReportInaccessibleFluidsNo,

This comment has been minimized.

@orod-org

orod-org May 27, 2018

CRITICAL Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'. rule

for( int x = 0; x < this.maxItemTypes; x++ )
{
itemSlots[x] = ITEM_SLOT + x;

This comment has been minimized.

@orod-org

orod-org May 27, 2018

MAJOR Remove this assignment of "itemSlots". rule

for( int x = 0; x < this.maxItemTypes; x++ )
{
itemSlots[x] = ITEM_SLOT + x;
itemSlotCount[x] = ITEM_SLOT_COUNT + x;

This comment has been minimized.

@orod-org

orod-org May 27, 2018

MAJOR Remove this assignment of "itemSlotCount". rule

if( o == null )
{
throw new AppEngException( "ItemStack was used as a cell, but was not a cell!" );

This comment has been minimized.

@orod-org

orod-org May 27, 2018

CRITICAL Define a constant instead of duplicating this literal "ItemStack was used as a cell, but was not a cell!" 3 times. rule

this.itemsPerByte = itemsPerByte;
if( itemSlots == null )
{
itemSlots = new String[this.maxItemTypes];

This comment has been minimized.

@orod-org

orod-org May 27, 2018

MAJOR Remove this assignment of "itemSlots". rule

if( itemSlots == null )
{
itemSlots = new String[this.maxItemTypes];
itemSlotCount = new String[this.maxItemTypes];

This comment has been minimized.

@orod-org

orod-org May 27, 2018

MAJOR Remove this assignment of "itemSlotCount". rule

@yueh yueh referenced this pull request May 27, 2018

Closed

Fluid Integration #3341

1 of 5 tasks complete
private final ContainerFluidTerminal container;
private final int offsetX = 9;
private int rows = 6;
private int maxRows = Integer.MAX_VALUE;

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Remove this unused "maxRows" private field. rule

return !( (IStorageCell) type ).storableInStorageCell();
}
}
catch( final Throwable err )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Either log or rethrow this exception. rule
MAJOR Catch Exception instead of Throwable. rule

{
final IAEItemStack toReturn = input.copy();
toReturn.setStackSize( input.getStackSize() - remainingItemCount );
if( mode == Actionable.MODULATE )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

CRITICAL Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule

final Item type = i.getItem();
if( type instanceof IStorageCell )
{
if ( ( (IStorageCell) type ).getChannel() == AEApi.instance().storage().getStorageChannel( IItemStorageChannel.class ) )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Merge this if statement with the enclosing one. rule

t = new ItemStack( compoundTag );
if( t.isEmpty() )
{
AELog.warn( "Removing item " + compoundTag + " from storage cell because the associated item type couldn't be found." );

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

CRITICAL Define a constant instead of duplicating this literal "Removing item " 3 times. rule

final long size = Math.min( Integer.MAX_VALUE, request.getStackSize() );
IAEItemStack Results = null;

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MINOR Rename this local variable to match the regular expression '^[a-z][a-zA-Z0-9]*$'. rule

return;
}
}
catch( Throwable ex )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Catch Exception instead of Throwable. rule

{
return new ItemCellInventoryHandler( new ItemCellInventory( o, container2 ) );
}
catch( final AppEngException e )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Either log or rethrow this exception. rule

}
@Override
public IAEItemStack injectItems( final IAEItemStack input, final Actionable mode, final IActionSource src )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

CRITICAL The Cyclomatic Complexity of this method "injectItems" is 17 which is greater than 10 authorized. rule

{
this.proxyable.getProxy().getTick().alertDevice( this.proxyable.getProxy().getNode() );
}
catch( GridAccessException ignore )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Either log or rethrow this exception. rule

{
this.cellItems.add( AEApi.instance().storage().getStorageChannel( IItemStorageChannel.class ).createStack( t ) );
}
catch( Throwable ex )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Catch Exception instead of Throwable. rule

return input;
}
// This is slightly hacky as it expects a read-only access, but fine for now.
// TODO: Guarantee a read-only access. E.g. provide an isEmpty() method and ensure CellInventory does not write

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

INFO Complete the task associated to this TODO comment. rule

* @version rv6 - 22/05/2018
* @since rv6 22/05/2018
*/
public class FluidSorters

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Add a private constructor to hide the implicit public one. rule

}
};
public static final Comparator<IAEFluidStack> CONFIG_BASED_SORT_BY_MOD = new Comparator<IAEFluidStack>()

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Reduce this anonymous class number of lines from 21 to at most 20, or make it a named class. rule

*/
public class FluidSorters
{
private static SortDir Direction = SortDir.ASCENDING;

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MINOR Rename this field "Direction" to match the regular expression '^[a-z][a-zA-Z0-9]*$'. rule

{
private static SortDir Direction = SortDir.ASCENDING;
public static final Comparator<IAEFluidStack> CONFIG_BASED_SORT_BY_NAME = new Comparator<IAEFluidStack>()

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Make this anonymous inner class a lambda rule

}
};
public static final Comparator<IAEFluidStack> CONFIG_BASED_SORT_BY_SIZE = new Comparator<IAEFluidStack>()

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Make this anonymous inner class a lambda rule

{
if( chan == AEApi.instance().storage().getStorageChannel( IFluidStorageChannel.class ) )
{
//TODO: Open Gui

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

INFO Complete the task associated to this TODO comment. rule

if( chan == AEApi.instance().storage().getStorageChannel( IFluidStorageChannel.class ) )
{
//TODO: Open Gui
//Platform.openGUI( player, (TileEntity) chest, AEPartLocation.fromFacing( chest.getUp() ), GuiBridge.GUI_ME );

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR This block of commented-out lines of code should be removed. rule

}
@Override
public void putStack( final ItemStack par1ItemStack )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

CRITICAL Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation. rule

@@ -289,6 +245,8 @@ private boolean disassembleDrive( final ItemStack stack, final World world, fina
return false;
}
protected abstract void dropEmptyStorageCellCase( final InventoryAdaptor ia, final EntityPlayer player );

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MINOR Redundant 'final' modifier. rule
MINOR Redundant 'final' modifier. rule

player.dropItem( extraA, false );
}
} );
dropEmptyStorageCellCase( ia, player );
if( player.inventoryContainer != null )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

CRITICAL Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule

BrockWS added some commits Jun 2, 2018

}
}
if( achievement != null && achievement.getActionableNode() != null )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Remove this expression which always evaluates to "true" rule

{
this.proxyable.getProxy().getTick().alertDevice( this.proxyable.getProxy().getNode() );
}
catch( GridAccessException ignore )

This comment has been minimized.

@orod-org

orod-org Jun 2, 2018

MAJOR Either log or rethrow this exception. rule

@orod-org

This comment has been minimized.

orod-org commented Jun 2, 2018

SonarQube analysis reported 231 issues

  • BLOCKER 13 blocker
  • CRITICAL 81 critical
  • MAJOR 95 major
  • MINOR 38 minor
  • INFO 4 info

Watch the comments in this conversation to review them.

Top 10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. BLOCKER GuiFluidStorageBus.java#: Add or update the header of this file. rule
  2. BLOCKER GuiFluidTerminal.java#: Add or update the header of this file. rule
  3. BLOCKER SlotFluidME.java#: Add or update the header of this file. rule
  4. BLOCKER ContainerFluidStorageBus.java#: Add or update the header of this file. rule
  5. BLOCKER ContainerFluidTerminal.java#: Add or update the header of this file. rule
  6. BLOCKER ISlotFluid.java#: Add or update the header of this file. rule
  7. BLOCKER PacketTargetFluidStack.java#: Add or update the header of this file. rule
  8. BLOCKER PartSharedStorageBus.java#: Add or update the header of this file. rule
  9. CRITICAL ContainerCellWorkbench.java#L99: Define a constant instead of duplicating this literal "config" 3 times. rule
  10. CRITICAL AEFeature.java#L99: Use already-defined constant 'CATEGORY_PORTABLE_CELL' instead of duplicating its value here. rule

@yueh yueh merged commit d166772 into AppliedEnergistics:rv6-1.12 Jun 11, 2018

2 of 3 checks passed

sonarqube SonarQube reported 231 issues, with 81 critical and 13 blocker
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment