Skip to content

Conversation

@ThatRedox
Copy link
Member

No description provided.

# Conflicts:
#	chunky/src/java/se/llbit/chunky/resources/Texture.java
#	chunky/src/java/se/llbit/chunky/resources/TexturePackLoader.java
@leMaik
Copy link
Member

leMaik commented Sep 6, 2024

This PR makes it so that Texture isn't even a texture anymore but just the list of hard-coded textures. Should we instead have an interface (or abstract class) Texture and then let texture types inherit from that?

The texture loaders and SimpleTexture (which isn't a texture but a texture loader, but that's outside of this PR's scope) now require BitmapTexture. Is that an actual requirement or would it be sufficient to require Texture? Maybe we should refactor the texture loaders from taking all parameters and a target Texture object to having a method that returns the texture. That way, the loader could select an appropriate Texture implementation.

@ThatRedox
Copy link
Member Author

This PR makes it so that Texture isn't even a texture anymore but just the list of hard-coded textures. Should we instead have an interface (or abstract class) Texture and then let texture types inherit from that?

Turning Texture into an interface might be a better solution. Originally I thought not changing texture would be less work (which wasn't the case at all).

The texture loaders and SimpleTexture (which isn't a texture but a texture loader, but that's outside of this PR's scope) now require BitmapTexture. Is that an actual requirement or would it be sufficient to require Texture?

The texture loaders need BitmapTexture to store the loaded texture into something, since not all textures support this (or should support it).

Maybe we should refactor the texture loaders from taking all parameters and a target Texture object to having a method that returns the texture. That way, the loader could select an appropriate Texture implementation.

This would probably be a better solution, but I'm not sure how it'd play out with having the list of hard coded textures. It'd probably involve a bigger refactor

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.

2 participants