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

Feature: Infinity Water Cell, Energy Card, Q Bridge Card, FDisplay Potion Support, uTerm Auto Stock and Network packet fix #189

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

asdflj
Copy link

@asdflj asdflj commented Jan 2, 2024

  • add energy card

  • add quantum bridge card

  • fluid terminal display potion effect support
    image

  • all terminal add data sync button

  • ultra terminal add automatic stock item
    动画1

  • add infinity water cell
    image

  • add infinity lava cell

  • network packet add gzip support

  • fix terminal sometime drop network packet

before after
动画2 动画3

add energy card
add quantum bridge card
fluid terminal display potion effect support
all terminal add data sync button
ultra terminal add automatic stock item
@asdflj asdflj marked this pull request as ready for review January 3, 2024 14:41
@Dream-Master Dream-Master requested review from Laiff and a team January 3, 2024 18:46
zy added 2 commits January 4, 2024 14:41
fix terminal sometime drop network packet
@Ethryan Ethryan changed the title Feature Feature: Energy Card, Q Bridge Card, FDisplay Potion Support, uTerm Auto Stock and Network packet fix Jan 4, 2024
@Ethryan
Copy link

Ethryan commented Jan 4, 2024

Just so the name isn't just feature.

add infinity lava cell
@asdflj asdflj changed the title Feature: Energy Card, Q Bridge Card, FDisplay Potion Support, uTerm Auto Stock and Network packet fix Feature: Infinity Water Cell, Energy Card, Q Bridge Card, FDisplay Potion Support, uTerm Auto Stock and Network packet fix Jan 4, 2024
@Laiff
Copy link

Laiff commented Jan 4, 2024

@asdflj Please stop force-push branch. When it in draft mode its fine but now it is very harmful to understand what was happened in this force push. And broke review that in progress

Comment on lines +44 to +66
this.drawTexturedModalRect(this.xPosition, this.yPosition, 0, 46 + k * 20, this.width / 2, this.height);
this.drawTexturedModalRect(
this.xPosition + this.width / 2,
this.yPosition,
200 - this.width / 2,
46 + k * 20,
this.width / 2,
this.height - 2);
this.drawTexturedModalRect(
this.xPosition + 2,
this.yPosition + (this.height - 2),
200 - (this.width - 2),
64 + k * 20,
this.width - 2,
2);
this.drawTexturedModalRect(
this.xPosition,
this.yPosition + (this.height - 1),
200 - this.width,
65 + k * 20,
this.width,
1);
this.mouseDragged(mc, mouseX, mouseY);
Copy link

Choose a reason for hiding this comment

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

Magic numbers

super.initGui();
this.buttonList.clear();

this.buttonList.add(
Copy link

Choose a reason for hiding this comment

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

same as in other split and use constants instead of magic numbers

@chochem chochem added the affects balance requires admin approval label Jan 4, 2024
Copy link
Member

@chochem chochem left a comment

Choose a reason for hiding this comment

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

Please add a lot more details for the added features (what do they do, how do they work, are there recipes, what is the proposed tiering), so that they can be discussed. (Ideally discussion should have been pre-PR)
Also it would be best if you make the bug fixes a separate PR.

Copy link

@Laiff Laiff left a comment

Choose a reason for hiding this comment

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

Overall interesting addition but it needs to be well documented and discussed, can you fill a proposal issue in main modpack issues with detailed explanation what why and how it works.
Also it would be great if you propose tiering for this items and possible gregified recipes.

src/main/java/com/glodblock/github/common/Config.java Outdated Show resolved Hide resolved
@SubscribeEvent
public void pickupEvent(EntityItemPickupEvent e) {
try {
if (Platform.isClient() || e.entityPlayer == null) return;
Copy link

Choose a reason for hiding this comment

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

This logic should be tested with magnet from draconic, only one should pick up items and do not produce duplicates

src/main/java/com/glodblock/github/proxy/CommonProxy.java Outdated Show resolved Hide resolved
src/main/java/com/glodblock/github/util/Util.java Outdated Show resolved Hide resolved
@asdflj
Copy link
Author

asdflj commented Jan 10, 2024

can we just merge it?

@Dream-Master
Copy link
Member

@Laiff

@Laiff
Copy link

Laiff commented Jan 11, 2024

What do you mean migrate? Move branch into base repository and finish it there or something else?

@chochem
Copy link
Member

chochem commented Jan 11, 2024

I dont know why you ask for a re-review, Dream. the author has not replied to anything yet.

They also continue to force-push after being asked to not do so :(

@asdflj
Copy link
Author

asdflj commented Jan 11, 2024

What do you mean migrate? Move branch into base repository and finish it there or something else?

merge

@Hikari1nVoid
Copy link

This is incomplete and should not be merged at all.No docs about new cells regarding tiers,recipes,etc..

@boubou19
Copy link
Member

@asdflj i'm afraid without any documentation and related ticket to discuss the balance we will never accept this PR.

@Caedis
Copy link
Member

Caedis commented Mar 10, 2024

honestly each thing should be in their own PR instead of all in one big one.

@Dream-Master
Copy link
Member

@Laiff said he will rework it when he got time

@Dream-Master Dream-Master added ongoing freeze - do not merge Not just a bug fix and thus affected by a current freeze for a upcoming version and removed ongoing freeze - do not merge Not just a bug fix and thus affected by a current freeze for a upcoming version labels Apr 28, 2024
@Quetz4l Quetz4l mentioned this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects balance requires admin approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants