-
Notifications
You must be signed in to change notification settings - Fork 23
Port to 1.21.7 #122
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
Port to 1.21.7 #122
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't been able to test it in-game yet but this all looks awesome. Seems like quite a lot had to be done for this port!
super.readData(view); | ||
|
||
this.title = view.getString("title", ""); | ||
this.url = view.getString("url", "glowcase:mod/modmenu"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new default? If so, that's great! Having a default helps explain the syntax (I would've never guessed it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the new data system gives an optional of t or t if you pass a default (unless it's a codec/read, that is always an optional with no way to pass a default, but you can use .orElse)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're curious of why the weird syntax: it was intended to be extended later on to allow mods to register directly initializing a specific screen, and I didn't want to marry it to subpar weirdness of just modmenu
being ambiguous.
Admittedly that should be a dedicated issue to discuss tho.
|
||
// Register image as texture | ||
TextureManager textureManager = MinecraftClient.getInstance().getTextureManager(); | ||
this.texture = textureManager.registerDynamicTexture("glowcase/img", nativeTexture); | ||
this.texture = Glowcase.id("glowcase/img", imageHash); | ||
textureManager.registerTexture(this.texture, nativeTexture); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change the behaviour? The first parameter used to be constant but is now different for each image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unsure on how it worked, and imagined minecraft assigned you an id for each image, if it was all in a single texture I can change it back
I should give it a look when I get to fixing the other bugs
src/main/java/dev/hephaestus/glowcase/client/render/block/entity/PopupBlockEntityRenderer.java
Show resolved
Hide resolved
isPlaying was evidently not sufficient for checking if the sound was also queued.
I'll mention; the datagen setup does not seem to be automated in any capacity yet, a clean build will be missing model definitions. |
oh yeah, it's not pushed to the repo, should I push it or should I set some sort of automation? |
You can push it, but if you know how to set up automation that would be a nice addition |
Reposting from the thread. Everything else is fine as far as I can tell. sound block: randomly selects one sound for multi-sound events and sticks to it rather than varying Since these are all minor issues, I think we're ready to ship. |
I'll keep it in draft for now as I want to make sure to test things first
TODO(fixes):