Added ImageHUD #5808
Added ImageHUD #5808
Conversation
There was a problem hiding this comment.
there are 2 extra major underlying issues with this pull request:
firstly, since the stored NativeImageBackedTexture is only destroyed inside ImageHud, deleting an ImageHud element from the hud editor causes its texture to be lost without closing, leaking both ram and vram
secondly, i highly dislike the usage of javax.imageio and java.awt for many reasons:
BufferedImageoperations are very expensive relative to minecraft'sNativeImagejava.awthas been known to cause many issues with minecraft in the past, it would be better to avoid touching it at allBufferedImageerrors are hard to diagnose :(
minecraft already has the necessary support for parsing jpegs, pngs, bmps, gifs, etc. through stbi, and you can easily modify NativeImages in the ways you need with NativeImage#fillRect and NativeImage#copyRect
| private static final Color TRANSPARENT = new Color(255, 255, 255, 255); | ||
| private static final int DEBOUNCE_TIME = 2; // 2 Seconds before rerunning. | ||
| private Identifier texture; | ||
| private final NameableExecutor worker = Util.getIoWorkerExecutor(); |
There was a problem hiding this comment.
executor should use daemon threads, Util#getIoWorkerExecutor does not daemon
|
|
||
| currentImageDataFuture = CompletableFuture.supplyAsync(() -> { | ||
| try { | ||
| InputStream imageFile = path.toLowerCase().startsWith("http") ? new URL(path).openStream() : new FileInputStream(parsed); |
There was a problem hiding this comment.
use a try-with-resources, both InputStream and ImageInputStream need to be closed
| return ImageDataFactory.fromStatic(name, reader); | ||
| } | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
an Exception may be thrown from the new URL(path) call, depending on the user input, it should be handled in some way (up to your discretion), not just be rethrown and logged as debug
| private static final int DEBOUNCE_TIME = 2; // 2 Seconds before rerunning. | ||
| private Identifier texture; | ||
| private final NameableExecutor worker = Util.getIoWorkerExecutor(); | ||
| private ImageData cachedImageData; |
There was a problem hiding this comment.
variables that are modified from offthread should be marked as volatile
| int frameIndex = getCurrentAnimationFrame(imageData.delays); | ||
| int row = frameIndex % imageData.framesPerColumn; | ||
| int column = frameIndex / imageData.framesPerColumn; | ||
| TextureRegion textureRegion = new TextureRegion(imageData.width,imageData.height); |
There was a problem hiding this comment.
since TextureRegion is mutable, it can easily be stored in ImageData and mutated rather than reallocated every frame
|
|
||
| import java.util.List; | ||
|
|
||
| public class ImageData { |
There was a problem hiding this comment.
i would say this should be a record, but since animated images require many more fields than static images, i think ImageData would be better off as a sealed interface with two record implementations
| * @return Identifier of the registered texture. | ||
| */ | ||
| public static NativeImage bufferedToNative(BufferedImage canvas) { | ||
| NativeImage tex = new NativeImage(canvas.getWidth(), canvas.getHeight(), true); |
There was a problem hiding this comment.
3rd parameter should be false since this NativeImage does not use stbi
| public static int getCurrentAnimationFrame(List<Integer> delays) { | ||
| int total = 0; | ||
| long time = Util.getMeasuringTimeMs() % delays.stream().mapToInt(d -> d * 10).sum(); | ||
| for (int i = 0; i < delays.size(); i++) { | ||
| total += delays.get(i) * 10; | ||
| if (time < total) return i; | ||
| } | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
- parameter should be
IntList - sum of delays should probably be calculated ahead of time when loading a gif and stored in
ImageData
|
I'm also unsure about the idea of packing a gif into a single texture, it sounds like that would either quickly hit some upper limit or take up a massive amount of vram |
|
LGTM 🚀🚀🚀 |
3 participants
Type of change
New feature
Description
A new HUD element that can draw still images and animated images from paths/urls in the in-game HUD
Related issues
Not related
How Has This Been Tested?
https://private-user-images.githubusercontent.com/51824929/499489242-eafb079f-3120-4a0a-a395-710eba5c37f6.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NjE1ODUxOTEsIm5iZiI6MTc2MTU4NDg5MSwicGF0aCI6Ii81MTgyNDkyOS80OTk0ODkyNDItZWFmYjA3OWYtMzEyMC00YTBhLWEzOTUtNzEwZWJhNWMzN2Y2LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTEwMjclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUxMDI3VDE3MDgxMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTIwODZjYjVhMzg1ZGZlNTQ1ZjQ3MTJmOTU5MDFiNjM3NzVhMGY1YzA4OGFkYTUzYmM5ZTEwYjI2NDYwOTg4ZDMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.uLKzeuOsSQi2bDrQsm1wq6Ngdl-ffWDvecIbce1UZy0
https://private-user-images.githubusercontent.com/51824929/499490549-0e8c5b15-a82c-4a0e-bce6-15f0de793787.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NjE1ODUxOTEsIm5iZiI6MTc2MTU4NDg5MSwicGF0aCI6Ii81MTgyNDkyOS80OTk0OTA1NDktMGU4YzViMTUtYTgyYy00YTBlLWJjZTYtMTVmMGRlNzkzNzg3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTEwMjclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUxMDI3VDE3MDgxMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTUwOGQ3MGRhNDk1ZmE1Zjg0NmM3NDM4ZjMwYjkzN2JhNjI3OTE3OTM5NWFhNmU4ZTIwMzFiOGJjMDBiNDEyOWQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.7CU1wvuNc6_i1yUjKb4RZEL6zfKf6aJB9630zqXdOiM
Checklist: