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

World rendering improvements #408

Merged
merged 11 commits into from
Dec 31, 2023
Merged

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Sep 18, 2023

Basically I picked almost all changes from my fork here. I've also dramatically changed the build pipeline (migrated everything to esbuild), but I didn't pick these changes here. So it won't be in sync.
I've separated changes by commits so I do hope it is still not hard to review :)

Added missing texture from invsprite from prismarine web client. Though it would be cool to think of a way to bring old behavior for some blocks if user really wants it for some reason.

Resourcepack support is done by supplying a custom atlas image generated with script like this. Of course would be better to think of exposing such functionality here, but it is not my priority for now. Also added a way to supply custom block state json so you can have any texture size (e.g. 1024x)

Also in prismarine web client I'm currently going to load textures and block states before joining the server or implement a loader indicator so user have understanding that textures are being loaded.

@rom1504
Copy link
Member

rom1504 commented Dec 17, 2023

hi, sorry for delay

CI is failing, could you make it work?

@rom1504
Copy link
Member

rom1504 commented Dec 17, 2023

I fixed the ci

function loadTexture (texture, cb) {
if (process.platform === 'browser') {
return require('./utils.web').loadTexture(texture, cb)
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? didn't it work before?

@@ -22,6 +27,9 @@ function loadTexture (texture, cb) {
}

function loadJSON (json, cb) {
if (process.platform === 'browser') {
return require('./utils.web').loadJSON(json, cb)
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? didn't it work before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As said in the body I moved to esbuild in my p viewer setup so I guess that's why I added it. If this is the only thing that blocks merging the pr and you don't want to see it you can remove it (feel free to do it since you should have push access)

texture.magFilter = THREE.NearestFilter
texture.minFilter = THREE.NearestFilter
texture.flipY = false
this.material.map = texture
})

loadJSON(`blocksStates/${version}.json`, blockStates => {
const loadBlockStates = () => {
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? didn't it work before?

Copy link
Contributor Author

@zardoy zardoy Dec 28, 2023

Choose a reason for hiding this comment

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

Still works, but now you can override logic of getting block states via exposed property so you can skip network request. Ideally it should also set to this property if wasn't set before after request, fixed in my version but not here but that's not a big issue imo. Overall it's not a bug fix, it's a customisation improvement

@rom1504
Copy link
Member

rom1504 commented Dec 17, 2023

Most things LGTM

I left 2 comments

@rom1504 rom1504 merged commit 4265ad7 into PrismarineJS:master Dec 31, 2023
18 checks passed
@rom1504
Copy link
Member

rom1504 commented Dec 31, 2023

/makerelease

@rom1504bot rom1504bot mentioned this pull request Dec 31, 2023
@zardoy
Copy link
Contributor Author

zardoy commented Jan 1, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants