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

[WIP] Update Spigot to 1.13 #331

Open
wants to merge 20 commits into
base: spigot-1.13
Choose a base branch
from
Open

Conversation

Katrix
Copy link

@Katrix Katrix commented Oct 17, 2018

Most stuff should be valid now, haven't done any tests yet. Still need to figure out what to do with some stuff.

Would be neat if I could get some info back on some of these.

  • How to handle stencils?
  • What in the world does BlockResetSurfaceBrush do?
  • How to handle slab staircase for spiral staircase?'
  • At the moment Ink no longer exists. Is there any realistic way to get it back?
  • Many of the block lists would be better served as custom tags. AFAIK, Spigot provides no way to provide custom tags. Would it be okay to use nms to add these?

Copy link
Member

@Deamon5550 Deamon5550 left a comment

Choose a reason for hiding this comment

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

The random changes of spaces to tabs and import reordering makes this quite hard to follow, please fix that.

The stencil brush should probably be updated to use schematics to be inline with the sponge version and generally more useful. There is a very old closed PR from me which did that, you could use it as a reference if you wanted to apply that change on top of this.

Ink would indeed be gone in the traditional way, but a much more powerful form could be made using the blockstates. Something like being able to specify rotation=north and apply that to all blocks which accept a rotation would replace the use that ink covered and be incredibly powerful.

I don't see any updates to the commands here, do the old commands still work with specifying the materials as numbers and data values? Also since you've removed some of the functions that the command call without updating them I'm guessing this doesn't actually compile?

@Katrix
Copy link
Author

Katrix commented Oct 18, 2018

Sorry for the whitespace changes. Didn't notice them before now. Will fix the import order too. Does the project have any official order? Looked around and didn't find any.

As for commands, I haven't done any changes there yet.

For ink, think that could be done, although don't know how fast it would be as it would mean a try-catch block for each performer call (could maybe reduce the overhead a bit with a cache) as Bukkit doesn't expose block traits in their raw form as far as I know.

Currently replaces stuff I'm fairly certain I haven't touched too. Will fix later
@Deamon5550
Copy link
Member

The import order is everything except java, and then java in a block afterwards.

I'd not worry about ink right now, that'll be a much more complicated change than warrants being rolled up into this change.

@gabizou
Copy link
Member

gabizou commented Jun 5, 2019

Any further work going to be done on updating this? If time permits, I just might sit down and update this in a month or two (if I'm being realistic, might end up sitting down to update end of summer given everything I have higher on my priority list).

@Katrix
Copy link
Author

Katrix commented Jun 5, 2019

Ah, sorry. Been getting sidetracked by other projects instead of this (mostly Ore). Don't want to say anything concretely, but I might still work some more on this in the summer. AFAIK, most stuff except stencils should be done (although I haven't tested anything). I don't think I have any uncommited stuff, but I can check later and commit them if I have.

@jaqobb
Copy link

jaqobb commented Jun 5, 2019

Any further work going to be done on updating this? If time permits, I just might sit down and update this in a month or two (if I'm being realistic, might end up sitting down to update end of summer given everything I have higher on my priority list).

This is not needed unless you really want to update the original repository. A member (@pitcer) from our team (MCParkour Development Team) is currently working on updating VoxelSniper to the newest version of Minecraft for our server's purposes. The current version already works for Minecraft 1.13 and we did not see any severe errors while testing it. The reason we did not create a pull request was mainly due to we wanted to change the code to fit our requirements so code style was changed and so as most of the code in general but functionality and features remained intact.

Feel free to take a look at our fork: voxel-sniper-flattened

@gabizou
Copy link
Member

gabizou commented Jun 5, 2019

This is not needed unless you really want to update the original repository.

It's better to have the original project updated. It's been an idea of ours to replace stencils with schematics as they're far more supported (stencils are such a legacy format at this point anyways). I can't attest to my interest in letting the project totally die, it's probably best to have the core updated for the overall public to benefit. There's already a few forks of VS that are floating around and all are quasi bugged in various ways from mumblings around various communities, so getting it right as the source project, I'd feel it's necessary to getting it updated.

@Katrix
Copy link
Author

Katrix commented Jul 13, 2019

Status update I guess. So, where is this at the moment? Almost done I think. It compiles, and runs fine on servers from the limited testing I've done. There's a few things left to be done. The tests are failing, and I don't know enough about maven to fix it. I also temporarily deleted the staircase brush, so that will have to be reimplemented. Finally I need to test that I implemented the schematics correctly.

@Deamon5550 Deamon5550 self-assigned this Oct 15, 2019
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.

5 participants