Image.getPixel/setPixel & ImagePainter – editing JME3 Images

That’s a much better approach, want me to take a crack at it?

@zarch said:
It's not very heavyweight I think (not much data stored, just code) so shouldn't be a big problem. That is one of the things I wanted to think about when I made the change though.

I was thinking that maybe ImagePainter should inherit (rather than encapsulate) from ImageRaster and all the parameters that currently take an Image should take an ImageRaster instead.

Then you could pass ImagePainters or ImageRasters to each other and you could re-use ImageRasters that you were using a lot rather than recreating them all the time.


I kind of disagree. For one thing, ImagePainter is not an ImageRaster no more than java.awt.Graphics is a BufferedImage. For another, if we ever make ImageRaster an interface or if there are different subclasses, etc., then your class hierarchy would get broken.

I think ImagePainter "has a" ImageRaster... that may be one of many different implementations.

Yeah, I was wondering about that myself…I think you are right that it’s better to stick with encapsulation… but I would still change the types to ImageRaster instead of Image. It does mean you can’t pass an ImagePainter to another ImagePainter, but it’s no big deal to do imagePainter.getRaster() instead.

True, the somewhat simpler code could cause problems down the road.



ImageRaster could swap out Image, but there were sections that needed the width/height, which I didn’t see a way to get from the raster. Not sure if having ImageRaster encapsulate its source Image is all that clean either…



Back to the original question then, and you might be able to answer better @pspeed, would creating an ImageRaster for each image copy be acceptable, performance-wise?

@novum feel free to have a crack at it. Keeping in mind the discussion I just had with @pspeed :smiley:



If you want once you’ve made the changes I can probably find some time tonight to have a look at them and investigate putting the completed file into a plugin for the SDK.

Damn, you are right - no get/set height in ImageRaster. It shouldn’t be a problem to add them though I expect since they make sense in the context of what an ImageRaster is - and the DefaultImageRaster which is the only current implementation contains an Image object so can just forward the calls to that.



@momoko_fan - any objection to adding getHeight and getWidth to ImageRaster?

Oh, there is AndroidImageInfo as well - but again that can just forward to bitmap.getHeight etc.

@novum said:
Back to the original question then, and you might be able to answer better @pspeed, would creating an ImageRaster for each image copy be acceptable, performance-wise?


I think this should be nearly free.

Looks pretty light. I’m getting around 10M calls in 500ms on my i7/2.8ghz - creating off of the same image, of course.

Your main risk is garbage collector hiccups from the deallocation of all those objects rather than the initial allocations. Not a big issue on desktop JVMs which are much smarter about it but it may be on Android.

Anyway, I agree to have the raster as the parameter and let the caller create them on the fly or manage them however else they want.

Assuming the answer to ImageRaster.getWidth()/getHeight() comes back yes and it’s in the near future - I can update ImagePainter and get you a diff to review @zarch. Your call.

Fine by me, we can’t do anything until we hear back from the core devs though.

Thanks, it was added to SVN.

@novum said:
Assuming the answer to ImageRaster.getWidth()/getHeight() comes back yes and it's in the near future - I can update ImagePainter and get you a diff to review @zarch. Your call.


Patch was done last night so the new feature should be there in either today or tomorrow's nightly build.
@Momoko_Fan said:
Thanks, it was added to SVN.


Thanks.

I’ll update the version I have here, test a bit, and then post a diff tonight/tomorrow - that work @zarch?

Sounds good to me. It will be nice to clear my patches out of JME and go back to using the standard releases.

Ok, usability judgement call. 8)



I went about removing Image from being encapsulated by ImagePainter and having it only encapsulate ImageRaster. Discovered it is still more functional to encapsulate Image - unless/until ImageRaster can retrieve its wrapped image in addition to getWidth/getHeight.



Why…



ImagePainter can construct with an Image, or create a new Image (how I’m using it). At some point after construction, I retrieve the Image and use it to setTexture on a material - via Texture2D( Image ).



The calling code could easily create the image itself, but then it is holding an Image and ImagePainter together - which seems odd.



It’s trivial either way - but figured I’d ask your preference since it’s your code.

Good question.



The problem is that ImageRaster doesn’t always wrap a JME3 “Image”.



The “clean” solution would have the user always pass in the ImageRaster to be rendered to but I think the more user-convenient solution is for ImagePainter to store both the created Image and the ImageRaster in the case where it creates the image. Then a query getCreatedImage() can be provided.