Skip to content
This repository has been archived by the owner on May 29, 2022. It is now read-only.

Few little optimisations #365

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

Conversation

crafter23456
Copy link
Contributor

@crafter23456 crafter23456 commented Feb 28, 2022

Description

Use Vanilla Minecart Speeds

  • Minecart is driving like it should, I didnt made any tests how far it fly etc only if it works normal

Optimize World Time Updates PaperMC/Paper@70e091b

  • time went normal, /time set X works. so should be fine

I bumped some dependencies and updated the velocity repo cause the link was broken. Log4j should be fixed with only the core in fact Paper did only the trove4j and log4j core in their 1.8.8 log4j bugfix.
PaperMC/Paper@2894af0#diff-9f5fad4c579c54b5c3b0080702b87b856679a932ee4338409dab8b8cebe06292R16
Besides of that I got inspired by the spigot 1.18.1 pom and changed gitdescribe to scriptus.
All builds fine. No console errors.

Checklist:

  • I have reviewed my code thoroughly.
  • I have tested my code.
  • My changes generate no new warnings

@HowardZHY
Copy link

will @crafter23456
Use Vanilla Minecart Speeds
have an influence with Train Carts pl ?

@crafter23456
Copy link
Contributor Author

will @crafter23456 Use Vanilla Minecart Speeds have an influence with Train Carts pl ?

dunno. it only changes the speed back to default when the cart is flying...

NachoSpigot-Server/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@HeathLoganCampbell HeathLoganCampbell left a comment

Choose a reason for hiding this comment

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

Personal I think this is a lot of changes in one pull request which makes things really messy, I would suggest breaking them up especially since dep bumping can be high risk

NachoSpigot-API/pom.xml Show resolved Hide resolved
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-iostreams</artifactId>
<version>2.17.1</version>
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say. Paper only use the log4j core and the other thing I don't remember currently but we have that too. And my anticheat has a fix for log4j which only activates if the server didn't done it before. Isn't a 100% trust but if paper do it, I guess

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though it would be safe, I don't think it's recommended to combine multiple versions of a library.
IMO, this should be reverted. There's no reason for this.

@crafter23456
Copy link
Contributor Author

okay thats quiet weird. on my fork it builds, and here not. in fact that it builded here until we commited entityhuman out...

@ghost
Copy link

ghost commented Mar 2, 2022

okay thats quiet weird. on my fork it builds, and here not. in fact that it builded here until we commited entityhuman out...

That’s because it can’t find bungeecord-chat and you have it cached

@Sculas
Copy link
Collaborator

Sculas commented Mar 2, 2022

Did you add that dependency? If not, we might have a problem again. Need to look into a Maven repository sometime for this.

@crafter23456
Copy link
Contributor Author

okay thats quiet weird. on my fork it builds, and here not. in fact that it builded here until we commited entityhuman out...

That’s because it can’t find bungeecord-chat and you have it cached

no i mean on my repo on GH with the same GH actions xd on my server yes thats cached

Did you add that dependency? If not, we might have a problem again. Need to look into a Maven repository sometime for this.

I changed the velocity repo link cause it was outdated and the normal build script but it builded here already until we added a commit which isnt related if the api doesnt get found

@ghost
Copy link

ghost commented Mar 2, 2022

Did you add that dependency? If not, we might have a problem again. Need to look into a Maven repository sometime for this.

It’s caused by the removal of the sonatype parent, I have a fix in crafter23456#3

@crafter23456
Copy link
Contributor Author

there still open conversations from heath

<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-iostreams</artifactId>
<version>2.17.1</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though it would be safe, I don't think it's recommended to combine multiple versions of a library.
IMO, this should be reverted. There's no reason for this.

@sadcenter
Copy link
Contributor

@Lucaskyy I personally like the time per player feature, and it has no performance impact so yea. I used time per player and it worked on nacho

@crafter23456
Copy link
Contributor Author

Personal I think this is a lot of changes in one pull request which makes things really messy, I would suggest breaking them up especially since dep bumping can be high risk

i split the boat thing into another PR. so now its not that much

@HowardZHY
Copy link

HowardZHY commented Mar 26, 2022 via email

@crafter23456
Copy link
Contributor Author

@HeathLoganCampbell I removed the new world time patch. Is there anything else I missed?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants