Closed John-Dean closed 1 year ago
I would also be tempted to retire the .url
method in the future, loading from .read
should be quicker and more flexible (can be done in WebWorkers for instance without any potential reliance on Canvas
/OffscreenCanvas
by Three).
I also can't imagine this would impact anyone as the only FileSystem included is the zip one (as mentioned) which doesn't support .url
. I had to actually build my own URLFileSystem
class for testing to even identify where the issue was with textures in the first place.
I would also be tempted to retire the
.url
method in the future, loading from.read
should be quicker and more flexible (can be done in WebWorkers for instance without any potential reliance onCanvas
/OffscreenCanvas
by Three).I also can't imagine this would impact anyone as the only FileSystem included is the zip one (as mentioned) which doesn't support
.url
. I had to actually build my ownURLFileSystem
class for testing to even identify where the issue was with textures in the first place.
What about just discard the current Texture
interface in block.ts file? Let's delegate all these operation to a interface TextureManager
. In this case, you can use url or whatever mapping from texture path to real texture
// perhapse we can have async method later?
export interface TextureManager {
hasTexture(path: string): boolean
loadTexture(path: string): Texture
}
-interface Texture {
- url: string;
- animation?: PackMeta.Animation;
-}
-type TextureRegistry = Record<string, Texture>;
- private loader = new TextureLoader();
- constructor(readonly textureRegistry: TextureRegistry, readonly option: { clipUVs?: boolean, modelOnly?: boolean } = {}) { }
+ constructor(readonly textureManager: TextureManager, readonly option: { clipUVs?: boolean, modelOnly?: boolean } = {}) { }
- } else if (texPath in textureRegistry) {
+ } else if (textureManager.hasTexture(texPath)) {
// build new material
+ const texture = textureManager.loadTexture(texPath);
- const tex = textureRegistry[texPath];
- const texture = this.loader.load(tex.url);
Thanks for the response @ci010
The only potential issue I can see is how I am currently using it to load chunk data (which I think is the intended method): (Minimal code example)
/*
- Loads a resource pack, world into memory.
- Gets the content of the chunk at (0,0) and then converts it to an array of models
*/
// Imports
import { ModelLoader, ResourceManager, ResourcePack, ResourceLocation } from "@xmcl/resourcepack"
import { BlockModelFactory } from "@xmcl/model";
import { openFileSystem } from "@xmcl/system"
import { WorldReader, RegionReader } from "@xmcl/world"
// Create resource manager
const resourceManager = new ResourceManager();
// Create model loader
const modelLoader = new ModelLoader(resourceManager);
// Create block model factory
const blockModelFactory = new BlockModelFactory(modelLoader.textures);
// Add resource pack
const resourcePackData = ZIPFILECONTENTSHERE;
const resourcePackFileSystem = await openFileSystem(resourcePackData)
const resourcePack = await ResourcePack.open(resourcePackFileSystem)
await resourceManager.addResourcePack(resourcePack)
// Create a world reader
const worldData = ZIPFILECONTENTSHERE;
const worldFileSystem = await openFileSystem(worldData)
const worldReader = new WorldReader(worldFileSystem)
// Load chunk data at (0,0)
const chunkData = await worldReader.getRegionData(0, 0)
const sections = chunkData.sections || (chunkData.Level || {}).Sections || []
const sectionData = sections.map((section) => {
const offsetY = section.Y * 16
const information = RegionReader.getSectionInformation(section) // New function added recently
const sectionBlockIds = RegionReader.getSectionBlockIdArray(section) // New function added recently
const palette = information.palette;
return { palette: palette, data: sectionBlockIds, y: offsetY }
})
// Loop over sections and get models
const models = []
await (sectionData.forEach(async (section) => {
// Palette to block states
const blockStates = await (section.palette.map(
async (item, index) => {
const blockInfo = section.palette[item]
const blockName = blockInfo.Name
try{
const blockStateFilePath = ResourceLocation.ofBlockStatePath(blockName) // New function added recently
const blockStateFile = await resourceManager.get(blockStateFilePath)
const blockState = JSON.parse(await blockStateFile.read("utf-8"))
return blockState
} catch (error) {
return {}
}
}
))
// Section into models
await (section.data.forEach(
async (id, index) => {
try{
const x = index & 15;
const y = (index >>> 8) & 15;
const z = (index >>> 4) & 15;
const blockInfo = section.palette[id]
const blockState = blockStates[id]
const options = blockInfo.Properties || {}
const variant = pickVariant(blockState, options) // This isn't in the library, implemented myself
const modelData = await modelLoader.loadModel(variant.model) // Changed function recently to cache model data
const blockModel = blockModelFactory.getObject(modelData, variant) // Changed function recently to use options
blockModel.position.x = x * 16
blockModel.position.y = (y * 16) + section.y
blockModel.position.z = z * 16
models.push(blockModel)
} catch (error) {} // This will error on blocks like minecraft:air or blocks not in the resource pack
}
))
}))
You would then do:
scene.add(...models)
to add all the models to the scene
So in this case a BlockModelFactory
is initialized with textureRegistry = modelLoader.textures
which is type Record<string, Resource>
I would be keen to keep this compatibility (as it seems this is how the modules are meant to slot together) and I think changing the type to the interface you mentioned (TextureManager
) breaks this
I realised my last comment is very long, basically it boils down to:
Currently the BlockModelFactory
textureRegistry
input (type: Record<string, Texture>
) is compatible with the ModelLoader().textures
property (type: Record<string, Resource>
)
aka: Texture
and Resource
are compatible
If we change the input we lose this compatibility. The justification as to why we would probably want to keep this is in my previous comment.
After some thoughts we could transition away from the 2 methods all together by:
.url
only is provided:
.read
becomes a fetch request for that URL, mimicing the behaviour of .read
from the Resource
type
.read
is then called, and loaded via the PR functionDataTexture
to MinecraftTexture
that has a read only .loaded
property that is the promise as aboveI'm happy to edit the PR to do this change if you're okay with that?
After some thoughts we could transition away from the 2 methods all together by:
- If
.url
only is provided:.read
becomes a fetch request for that URL, mimicing the behaviour of.read
from theResource
type.read
is then called, and loaded via the PR function- We expand Three.js'
DataTexture
toMinecraftTexture
that has a read only.loaded
property that is the promise as aboveI'm happy to edit the PR to do this change if you're okay with that?
My key consern for this PR is it adds a hard-coded image decode libray. The suggestions I made are also intent to be decouple the decoding & loading process so users can decode by themself.
I guess I still prefer to create a TextureManager interface like I mentioned above
export interface TextureManager {
hasTexture(path: string): boolean
loadTexture(path: string): Texture
}
You can add some default implementation in your project code like
export class SimpleTextureHandler implements TextureManager {
constructor(private textures: Record<string, Resource> = {}) {}
hasTexture(path: string): boolean {
return !!this.textures[path]
}
loadTexture(path: string): Texture {
// Put your load & decode texture logic here
}
}
I'm okay for breaking change here as this package features are experimental.
No worries, I'm happy with this and I'll see what I can do on the PR to adapt to this change.
The PR should now implement it, and I've changed the demo code to reflect this change as well. Just going to do some thorough testing
The existing function to load URLs is limited, basically calling
THREE.TextureLoader.load(textureData.url)
with no callback watching, and no support when the file system doesn't support URLs (i.e. .zip) - which is the only file system included by default.This PR attempts to resolve this by moving the texture loading into a new function that mimics the current behaviour when a URL is present, but in the case when a URL is not present attempts to parse the texture file contents into a
ImageData
object which can be used as a texture. To do this a dependancy image-in-browser has been added to decode the images intoImageData
objects.Also added in this PR is a promise attached to the texture's
.userData
property (texture.userData.hasLoaded
) that resolves when a texture has loaded and rejects on error (i.e. bad URL or data). Previously the errors were silent. To note, this does go against THREE.js guidance as functions/promises should not be stored in userData as they will not be cloned, but this seemed like the best workaround that I could come up with to track if a texture has loaded.In summary adds a new texture load function that:
texture.userData.hasLoaded
)