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

Added an option to toggle oredict substitutions for patterns. #1891

Merged
merged 1 commit into from Oct 10, 2015

Conversation

Projects
None yet
3 participants
@yueh
Member

yueh commented Sep 21, 2015

It adds a backward compatibility to convert current patterns to use
oredict by default, which should be removed with rv4 stable.

Closes #1156

@yueh yueh changed the title from Added an option to toggle oredict subsitutions for patterns. to Added an option to toggle oredict substitutions for patterns. Sep 21, 2015

@@ -158,6 +158,10 @@ else if( this.Name.equals( "PatternTerminal.Clear" ) )
{
cpt.clear();
}
else if( this.Name.equals( "PatternTerminal.Substitute" ) )

This comment has been minimized.

@orod-org

orod-org Sep 21, 2015

MAJOR Move the "PatternTerminal.Substitute" string literal on the left side of this string comparison. rule

@orod-org

orod-org Sep 21, 2015

MAJOR Move the "PatternTerminal.Substitute" string literal on the left side of this string comparison. rule

@@ -158,6 +158,10 @@ else if( this.Name.equals( "PatternTerminal.Clear" ) )
{
cpt.clear();
}
else if( this.Name.equals( "PatternTerminal.Substitute" ) )
{
cpt.ct.setSubstitution( this.Value.equals( "1" ) );

This comment has been minimized.

@orod-org

orod-org Sep 21, 2015

MAJOR Move the "1" string literal on the left side of this string comparison. rule

@orod-org

orod-org Sep 21, 2015

MAJOR Move the "1" string literal on the left side of this string comparison. rule

boolean sim;
private int bytes = 0;
private boolean canEmit = false;
private boolean cannotUse = false;

This comment has been minimized.

@orod-org

orod-org Sep 21, 2015

MAJOR Remove this unused "cannotUse" private field. rule

@orod-org

orod-org Sep 21, 2015

MAJOR Remove this unused "cannotUse" private field. rule

@@ -54,7 +54,7 @@
LevelType, LevelType_Energy, LevelType_Item, InventoryTweaks, TerminalStyle, TerminalStyle_Full, TerminalStyle_Tall, TerminalStyle_Small,
Stash, StashDesc, Encode, EncodeDescription, Substitutions, SubstitutionsOn, SubstitutionsOff, SubstitutionsDesc, CraftOnly, CraftEither,
Stash, StashDesc, Encode, EncodeDescription, Substitutions, SubstitutionsOn, SubstitutionsOff, SubstitutionsDescEnabled, SubstitutionsDescDisabled, CraftOnly, CraftEither,

This comment has been minimized.

@orod-org

orod-org Sep 21, 2015

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

@orod-org

orod-org Sep 21, 2015

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

And,
With,
Substitute,
Yes,

This comment has been minimized.

@orod-org

orod-org Sep 21, 2015

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

@orod-org

orod-org Sep 21, 2015

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

With,
Substitute,
Yes,
No,

This comment has been minimized.

@orod-org

orod-org Sep 21, 2015

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

@orod-org

orod-org Sep 21, 2015

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

Yes,
No,
MolecularAssembler,

This comment has been minimized.

@orod-org

orod-org Sep 21, 2015

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

@orod-org

orod-org Sep 21, 2015

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

@yueh yueh added this to the rv3 - 1.7.10 milestone Sep 22, 2015

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Sep 23, 2015

Member

I am also not fully convinced about how to handle the backward compatibility.
Should we consider any "old" pattern as oredict enabled or not. According to the existing code in PatternHelper.canSubstitute(), they were actually not intended to use any substitute.

Member

yueh commented Sep 23, 2015

I am also not fully convinced about how to handle the backward compatibility.
Should we consider any "old" pattern as oredict enabled or not. According to the existing code in PatternHelper.canSubstitute(), they were actually not intended to use any substitute.

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Sep 23, 2015

Member

I think oredict should be off by default.

Member

thatsIch commented Sep 23, 2015

I think oredict should be off by default.

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Sep 23, 2015

Member

I also tend to this option, but will potentially cause more issues to players when upgrading and their tendency to ignore changelogs.

Member

yueh commented Sep 23, 2015

I also tend to this option, but will potentially cause more issues to players when upgrading and their tendency to ignore changelogs.

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Sep 23, 2015

Member

ah, the old pattern did support oredict right,
then 1.7 on by default with a //TODO marker?

Member

thatsIch commented Sep 23, 2015

ah, the old pattern did support oredict right,
then 1.7 on by default with a //TODO marker?

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Sep 23, 2015

Member

Old patterns are supporting oredict as it was the only option. But codewise it seems they should not.

New patterns will now have a new nbttag, but nonexistant nbttags will by default fallback to false, so it currently checks if the tag exists and if not will fallback to true instead.
It would be possible to simply negate it at one point, but this is really clunky, if the code works with "can", but stored as "cannot".

Also there is already a TODO for the pattern helper to remove it at one point.
If completely new patterns then use oredict or not by default is not really important. I honestly have currently even no idea what the default option is.

Member

yueh commented Sep 23, 2015

Old patterns are supporting oredict as it was the only option. But codewise it seems they should not.

New patterns will now have a new nbttag, but nonexistant nbttags will by default fallback to false, so it currently checks if the tag exists and if not will fallback to true instead.
It would be possible to simply negate it at one point, but this is really clunky, if the code works with "can", but stored as "cannot".

Also there is already a TODO for the pattern helper to remove it at one point.
If completely new patterns then use oredict or not by default is not really important. I honestly have currently even no idea what the default option is.

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Sep 23, 2015

Member

I think new patterns should have ore dict off per default.
You could also add a transition item with a text like "put into bench to convert to new" or so like the old TE pipes in 1.6

Might be the cleanest solution from player perspective

Member

thatsIch commented Sep 23, 2015

I think new patterns should have ore dict off per default.
You could also add a transition item with a text like "put into bench to convert to new" or so like the old TE pipes in 1.6

Might be the cleanest solution from player perspective

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Sep 23, 2015

Member

A transition item would be extremely intrusive to existing builds as this would mean update every single pattern.

Disabling it would mean only recreate needed ones, but still will require some player interaction.

Enabling will keep the current behaviour, but need like 5 lines of code extra to handle it.

Member

yueh commented Sep 23, 2015

A transition item would be extremely intrusive to existing builds as this would mean update every single pattern.

Disabling it would mean only recreate needed ones, but still will require some player interaction.

Enabling will keep the current behaviour, but need like 5 lines of code extra to handle it.

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Sep 26, 2015

Member

I slowly tend towards disabling it even for old patterns. The crafting system is already designed around precise items when calculating the crafting tree and most requests are about disabling this feature in general.

I really cannot remember any real feedback regarding it is an actually used very frequently, But the positive feedback is way more rare then negative ones.
Also the amount of patterns requiring oredict enable is probably way less then these requiring it disabled, so I would assume it will be less work for the players actually and I have no issue about closing a couple of issues and linking the changelogs.

Member

yueh commented Sep 26, 2015

I slowly tend towards disabling it even for old patterns. The crafting system is already designed around precise items when calculating the crafting tree and most requests are about disabling this feature in general.

I really cannot remember any real feedback regarding it is an actually used very frequently, But the positive feedback is way more rare then negative ones.
Also the amount of patterns requiring oredict enable is probably way less then these requiring it disabled, so I would assume it will be less work for the players actually and I have no issue about closing a couple of issues and linking the changelogs.

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Sep 26, 2015

Member

sounds about right

Member

thatsIch commented Sep 26, 2015

sounds about right

Show outdated Hide outdated src/api/java/appeng/api/config/Substitution.java
package appeng.api.config;
public enum Substitution

This comment has been minimized.

@thatsIch

thatsIch Oct 6, 2015

Member

should probably state for what usage this is supposed to be used. Sometimes there are duplicate classes which do nearly the same but are used in different contextes and to obtain type-safety they use different ones.

@thatsIch

thatsIch Oct 6, 2015

Member

should probably state for what usage this is supposed to be used. Sometimes there are duplicate classes which do nearly the same but are used in different contextes and to obtain type-safety they use different ones.

Show outdated Hide outdated src/main/java/appeng/client/gui/implementations/GuiPatternTerm.java
if( this.substitutionsEnabledBtn == btn || this.substitutionsDisabledBtn == btn )
{
NetworkHandler.instance.sendToServer( new PacketValueConfig( "PatternTerminal.Substitute", this.substitutionsEnabledBtn == btn ? "0" : "1" ) );

This comment has been minimized.

@thatsIch

thatsIch Oct 6, 2015

Member

should probably const the 0 and the 1 out to clarify meaning

@thatsIch

thatsIch Oct 6, 2015

Member

should probably const the 0 and the 1 out to clarify meaning

This comment has been minimized.

@yueh

yueh Oct 6, 2015

Member

Might be even an idea to refactor PacketValueConfig to also accept an enum instead of just a random string. Serialization probably still has to fall back to .ordinal() or similar.

@yueh

yueh Oct 6, 2015

Member

Might be even an idea to refactor PacketValueConfig to also accept an enum instead of just a random string. Serialization probably still has to fall back to .ordinal() or similar.

this.substitutionsEnabledBtn.visible = false;
this.substitutionsDisabledBtn.visible = true;
}

This comment has been minimized.

@thatsIch

thatsIch Oct 6, 2015

Member

just thinking as enhancement:

A class ToggleGroup or so, and if you make 1 button visible, all others are disabled via listener. Would be probably interessting in other button groups, so adding a new state is far easier than hard-coding the state.

@thatsIch

thatsIch Oct 6, 2015

Member

just thinking as enhancement:

A class ToggleGroup or so, and if you make 1 button visible, all others are disabled via listener. Would be probably interessting in other button groups, so adding a new state is far easier than hard-coding the state.

This comment has been minimized.

@yueh

yueh Oct 6, 2015

Member

Good idea. The whole gui could see some improvments. But simply not enough time currently to also refactor this.

@yueh

yueh Oct 6, 2015

Member

Good idea. The whole gui could see some improvments. But simply not enough time currently to also refactor this.

Show outdated Hide outdated src/main/java/appeng/container/implementations/ContainerPatternTerm.java
if( this.substitute != this.ct.isSubstitution() )
{
this.substitute = this.ct.isSubstitution();
}

This comment has been minimized.

@thatsIch

thatsIch Oct 6, 2015

Member

so its always this.substitute = this.ct.isSubstitution();? any performance implications why the ifis used?

@thatsIch

thatsIch Oct 6, 2015

Member

so its always this.substitute = this.ct.isSubstitution();? any performance implications why the ifis used?

This comment has been minimized.

@yueh

yueh Oct 6, 2015

Member

Hm yeah. Mostly pointless. I think I left it intentionally there in case it needed something similar to the crafting mode and updateOrderOfOutputSlots().

@yueh

yueh Oct 6, 2015

Member

Hm yeah. Mostly pointless. I think I left it intentionally there in case it needed something similar to the crafting mode and updateOrderOfOutputSlots().

Show outdated Hide outdated src/main/java/appeng/crafting/CraftingTreeNode.java
if( this.parent.details.canSubstitute() )
{
itemList = inv.getItemList().findFuzzy( this.what, FuzzyMode.IGNORE_ALL );

This comment has been minimized.

@thatsIch

thatsIch Oct 6, 2015

Member

inv.getItemList() used in both branches

@thatsIch

thatsIch Oct 6, 2015

Member

inv.getItemList() used in both branches

Show outdated Hide outdated src/main/java/appeng/helpers/PatternHelper.java
{
ACCEPT, DECLINE, TEST
}
static class TestLookup
private static class TestLookup

This comment has been minimized.

@thatsIch

thatsIch Oct 6, 2015

Member

final?

@thatsIch

thatsIch Oct 6, 2015

Member

final?

Added an option to toggle oredict subsitutions for patterns.
It adds a backward compatibility to convert current patterns to use
oredict by default, which should be removed with rv4 stable.

Closes #1156
@orod-org

This comment has been minimized.

Show comment
Hide comment
@orod-org

orod-org Oct 6, 2015

SonarQube analysis reported 9 issues:

  • MAJOR 4 major
  • MINOR 5 minor

Watch the comments in this conversation to review them.
Note: the following issues could not be reported as comments because they are located on lines that are not displayed in this pull request:

orod-org commented Oct 6, 2015

SonarQube analysis reported 9 issues:

  • MAJOR 4 major
  • MINOR 5 minor

Watch the comments in this conversation to review them.
Note: the following issues could not be reported as comments because they are located on lines that are not displayed in this pull request:

public final String Name;
public final String Value;
private final String Name;
private final String Value;

This comment has been minimized.

@thatsIch

thatsIch Oct 8, 2015

Member

vars should start lower case and not like classes

@thatsIch

thatsIch Oct 8, 2015

Member

vars should start lower case and not like classes

This comment has been minimized.

@yueh

yueh Oct 8, 2015

Member

I know.. There are a bunch of them. Probably something I will tackle should the PR for private fields be merged. Also the whole PacketValueConfig is a bit cheesy. Something I want to address with the network refactoring.

@yueh

yueh Oct 8, 2015

Member

I know.. There are a bunch of them. Probably something I will tackle should the PR for private fields be merged. Also the whole PacketValueConfig is a bit cheesy. Something I want to address with the network refactoring.

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Oct 8, 2015

Member

Looks fine, I will live test it tomorrow

Member

thatsIch commented Oct 8, 2015

Looks fine, I will live test it tomorrow

@thatsIch thatsIch self-assigned this Oct 8, 2015

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Oct 10, 2015

Member

A nice thing would be if you could see a difference in oredict vs not. This can be another FR though

Member

thatsIch commented Oct 10, 2015

A nice thing would be if you could see a difference in oredict vs not. This can be another FR though

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Oct 10, 2015

Member

Upon crafting my client crashes

Member

thatsIch commented Oct 10, 2015

Upon crafting my client crashes

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Oct 10, 2015

Member

Tried several times already, still failing

Member

thatsIch commented Oct 10, 2015

Tried several times already, still failing

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Oct 10, 2015

Member

This still crashes without NEI and CC

Member

thatsIch commented Oct 10, 2015

This still crashes without NEI and CC

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Oct 10, 2015

Member

Another thing is the place of the substitution button, not sure if this should be there, but maybe below the crafting/processing switch?

Member

thatsIch commented Oct 10, 2015

Another thing is the place of the substitution button, not sure if this should be there, but maybe below the crafting/processing switch?

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Oct 10, 2015

Member

The button was already there. Just hidden, so I simply kept it.

Different icon? As the tooltip already shows if enabled or not and this is also visible inside an interface terminal.

Regarding the crash see: https://bugs.mojang.com/browse/MC-32606

Member

yueh commented Oct 10, 2015

The button was already there. Just hidden, so I simply kept it.

Different icon? As the tooltip already shows if enabled or not and this is also visible inside an interface terminal.

Regarding the crash see: https://bugs.mojang.com/browse/MC-32606

@thatsIch

This comment has been minimized.

Show comment
Hide comment
Member

thatsIch commented Oct 10, 2015

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Oct 10, 2015

Member

Using the super latest driver fixes this issue or force not to use your 2D desktop application GFX-card

Member

thatsIch commented Oct 10, 2015

Using the super latest driver fixes this issue or force not to use your 2D desktop application GFX-card

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Oct 10, 2015

Member

I mean a direct indicator like upon shift show an iron ingot in the corner to indicate that these are oredictionaried.

Member

thatsIch commented Oct 10, 2015

I mean a direct indicator like upon shift show an iron ingot in the corner to indicate that these are oredictionaried.

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Oct 10, 2015

Member

Hm. There is just the general issue with only 16x16 pixel large icons.

But probably another FR. Also not sure if still possible with 1.8/1.9. I do not really want to add something, if we already know it needs to be reverted later.

Member

yueh commented Oct 10, 2015

Hm. There is just the general issue with only 16x16 pixel large icons.

But probably another FR. Also not sure if still possible with 1.8/1.9. I do not really want to add something, if we already know it needs to be reverted later.

yueh added a commit that referenced this pull request Oct 10, 2015

Merge pull request #1891 from yueh/feature-1156
Added an option to toggle oredict substitutions for patterns.

@yueh yueh merged commit 5643519 into AppliedEnergistics:master Oct 10, 2015

3 checks passed

default Finished TeamCity Build Applied Energistics :: Pull Requests : Tests passed: 53
Details
jenkins Success 53 tests run, 0 skipped, 0 failed.
Details
sonarqube SonarQube reported 9 issues, no critical nor blocker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment