PIXEL TIME!!!
2 Likes
@madjack said:
It is a nice idea and it works when everyone is on the same page. In this case we had something, not nothing. Zarch had his modifications and they were working (afaik). So instead of dialoguing with what was made, Kirill went off without much of an explanation and rewrote the whole thing.
There are no real loser in the end as the feature gets implemented and available for anyone who might need it. There's only a loss of time, which can be frustrating.
When you add too much friction to the process of moving forward then forward movement stops. None of us are paid to work on JME so we use our limited free time to do things. The trade offs are not always pretty (as in this case where the conversation would have taken 10x longer than just doing it another way).
Early communication probably could have been better but that's all water under the bridge now. To me, the code is the only real discussion because it leaves little room for interpretation.
Calmer heads need to come back at the discussion at the technical level to point out real design problems. The idiomatic ones need to be left aside, I think.
It is, indeed, time to move on.
I’ll second that motion.
androlo said:
PIXEL TIME!!!!!!
What he said :)
@zarch Your code samples at least made me very enthusiastic and started me thinking about all the sweet things I could do with image painting, that you should take with you, and also if it wasn't for your work I suspect set/getPixel wouldn't have happend so soon :)
No grudges please, that kills the soul. But keep the technical debate going and in the end jME will have the best raster-graphics code of them all. This will be so much fun!
1 Like
This isn’t intended as an attack on @momoko_fan, however I’ve done the performance tests I mentioned previously. The good news is that I rendered the outputs of the speed test to screen and both old and new looked identical.
Here is the code, if anyone spots any mistakes please let me know.
[java]
Format[] formats = new Format[] {
Format.ABGR8,
Format.BGR8,
Format.Luminance8,
Format.RGBA8,
Format.RGB8,
};
EnumMap<Format, Image> oldSourceImages = new EnumMap<Format, Image>(Format.class);
EnumMap<Format, Image> oldDestImages = new EnumMap<Format, Image>(Format.class);
EnumMap<Format, Image> newSourceImages = new EnumMap<Format, Image>(Format.class);
EnumMap<Format, Image> newDestImages = new EnumMap<Format, Image>(Format.class);
private void testOld(Image source, Image dest) {
for (int y=0;y<source.getHeight();y++)
for (int x=0;x<source.getWidth();x++)
dest.setPixel(x, y, source.getPixel(x, y));
}
private void testNew(Image source, Image dest) {
ImageRaster sourceRaster = new ImageRaster(source);
ImageRaster destRaster = new ImageRaster(dest);
for (int y=0;y<source.getHeight();y++)
for (int x=0;x<source.getWidth();x++)
destRaster.setPixel(x, y, sourceRaster.getPixel(x, y));
}
private void speedTest() {
ImagePainter sourcePainter = new ImagePainter(Format.RGBA8, 128, 128);
sourcePainter.paintGradient(0, 0, 128, 128, ColorRGBA.Red, ColorRGBA.Green, ColorRGBA.Blue, ColorRGBA.Black, BlendMode.SET);
Image image = sourcePainter.getImage();
// Call testOld and new each 50 times to “warm up”
for (Format format: formats) {
Image src = new ImagePainter(format, 128, 128).getImage();
oldSourceImages.put(format, src);
Image dest = new ImagePainter(format, 128, 128).getImage();
oldDestImages.put(format, dest);
for (int i=0;i<10;i++) {
testOld(image, src);
}
src = new ImagePainter(format, 128, 128).getImage();
newSourceImages.put(format, src);
dest = new ImagePainter(format, 128, 128).getImage();
newDestImages.put(format, dest);
for (int i=0;i<10;i++) {
testNew(image, src);
}
}
// Run tests
for (Format sourceFormat: formats) {
for (Format destFormat: formats) {
logger.log(Level.INFO, “Transcode {0} to {1}”, new Object[]{sourceFormat, destFormat});
long start = System.nanoTime();
for (int i=0;i<100;i++)
testOld(oldSourceImages.get(sourceFormat), oldDestImages.get(destFormat));
long oldTime = System.nanoTime() - start;
start = System.nanoTime();
for (int i=0;i<100;i++)
testNew(oldSourceImages.get(sourceFormat), newDestImages.get(destFormat));
long newTime = System.nanoTime() - start;
logger.log(Level.INFO, “Time taken: Old {0} ({1}) New {2} ({3})”, new Object[]{oldTime, oldTime/1000, newTime, newTime/1000});
}
}
}
[/java]
I’ve tried to be as fair as possible. The only potential improvement/bias to be made in favour of the new system that I can see would be to removing the creation of a new raster object and re-using one for each test. I doubt that is having a serious impact though and it does reflect real-world usage scenario whereas removing it most likely wouldn’t.
The…lets say potentially-conflict-causing…news is:
[java]
Aug 16, 2012 5:11:11 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode ABGR8 to ABGR8
Aug 16, 2012 5:11:11 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 105,549,584 (105,549) New 153,620,898 (153,620)
Aug 16, 2012 5:11:11 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode ABGR8 to BGR8
Aug 16, 2012 5:11:12 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 96,848,669 (96,848) New 144,022,125 (144,022)
Aug 16, 2012 5:11:12 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode ABGR8 to Luminance8
Aug 16, 2012 5:11:12 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 86,596,389 (86,596) New 133,868,306 (133,868)
Aug 16, 2012 5:11:12 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode ABGR8 to RGBA8
Aug 16, 2012 5:11:12 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 102,276,346 (102,276) New 184,411,059 (184,411)
Aug 16, 2012 5:11:12 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode ABGR8 to RGB8
Aug 16, 2012 5:11:12 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 97,025,780 (97,025) New 140,831,439 (140,831)
Aug 16, 2012 5:11:12 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode BGR8 to ABGR8
Aug 16, 2012 5:11:13 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 133,692,097 (133,692) New 146,695,589 (146,695)
Aug 16, 2012 5:11:13 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode BGR8 to BGR8
Aug 16, 2012 5:11:13 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 130,636,495 (130,636) New 135,528,339 (135,528)
Aug 16, 2012 5:11:13 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode BGR8 to Luminance8
Aug 16, 2012 5:11:13 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 115,828,581 (115,828) New 129,510,795 (129,510)
Aug 16, 2012 5:11:13 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode BGR8 to RGBA8
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 130,859,534 (130,859) New 175,749,769 (175,749)
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode BGR8 to RGB8
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 130,828,315 (130,828) New 136,619,218 (136,619)
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode Luminance8 to ABGR8
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 83,755,421 (83,755) New 126,418,271 (126,418)
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode Luminance8 to BGR8
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 79,573,819 (79,573) New 122,581,883 (122,581)
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode Luminance8 to Luminance8
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 70,394,407 (70,394) New 116,217,622 (116,217)
Aug 16, 2012 5:11:14 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode Luminance8 to RGBA8
Aug 16, 2012 5:11:15 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 85,408,850 (85,408) New 165,937,262 (165,937)
Aug 16, 2012 5:11:15 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode Luminance8 to RGB8
Aug 16, 2012 5:11:15 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 81,212,538 (81,212) New 118,821,443 (118,821)
Aug 16, 2012 5:11:15 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGBA8 to ABGR8
Aug 16, 2012 5:11:15 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 100,232,975 (100,232) New 183,994,400 (183,994)
Aug 16, 2012 5:11:15 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGBA8 to BGR8
Aug 16, 2012 5:11:15 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 95,070,062 (95,070) New 183,163,782 (183,163)
Aug 16, 2012 5:11:15 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGBA8 to Luminance8
Aug 16, 2012 5:11:16 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 84,779,959 (84,779) New 172,030,455 (172,030)
Aug 16, 2012 5:11:16 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGBA8 to RGBA8
Aug 16, 2012 5:11:16 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 101,094,211 (101,094) New 222,361,575 (222,361)
Aug 16, 2012 5:11:16 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGBA8 to RGB8
Aug 16, 2012 5:11:16 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 96,884,691 (96,884) New 177,375,581 (177,375)
Aug 16, 2012 5:11:16 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGB8 to ABGR8
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 95,865,557 (95,865) New 140,795,717 (140,795)
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGB8 to BGR8
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 91,865,568 (91,865) New 134,725,340 (134,725)
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGB8 to Luminance8
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 78,967,142 (78,967) New 125,400,637 (125,400)
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGB8 to RGBA8
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 95,643,720 (95,643) New 176,044,252 (176,044)
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Transcode RGB8 to RGB8
Aug 16, 2012 5:11:17 PM jme3test.texture.ImageAndImagePainter speedTest
INFO: Time taken: Old 89,781,070 (89,781) New 132,468,836 (132,468)
[/java]
I’m seeing a pretty consistent slowdown of between 50% and 80% from the old system to the new.
See? The code wasn’t wasted time 
I’ll let others deal with the performance implications because I haven’t had time to drill in and see where the time has gone. However, from a design perspective, I have some things I wish were different about the new code… truthfully, I also had some things I wished were different about the other code.
All of these implementations have locked us into a situation where the raster only supports what’s in core. If you want to support some other raster then you are out of luck… you either have to hack an enum with new functionality or badly subclass a concrete ImageRaster class. And to me this isn’t just a problem for end users but a sign that the core is tightly coupled to its own limitations and that just rubs me wrong in general.
So, here is roughly how I wish it was done. I’ve used the interface + static factory idiom because it is extremely common these days but names can be subject to change.
[java]
public interface Raster {
public ColorRGBA getPixel(int x, int y, ColorRGBA store);
public void setPixel(int x, int y, ColorRGBA color);
}
[/java]
[java]
public class Rasters {
public static Raster create( Image img ) {
…create a raster implementation specifically for the image…
}
…maybe some other factory methods we can’t foresee right now…
}
[/java]
This frees up the code to go in a variety of different directions… moreover, end users can create their own custom Raster classes if they want and easily swap them out with others.
It also makes things more flexible if we ever want to support additional methods on Raster for dealing with regions, etc… The static factory code can then adapt the implementation however it needs.
It also makes it more flexible to create slower easy to write implementations while still allowing tailored highly format-specific implementations tomorrow. For example, it’s no problem to have one separate concrete Raster implementation class per format if that turns out to be performance-desirable over the extra maintenance burden. So today we can create a catch-all single-class implementation with lots of branches and fetches… and tomorrow split that out into a few, more performant, implementations.
This also might be a nice doorway to incorporate some of the optimizations in zarch’s implementation. I don’t know… because as I’ve said I didn’t really dive in to find out where the cycles went.
1 Like
Yes, the static factory pattern would make sense in this case, particularly with breaking it out into ImageRaster instead of leveraging the enum.
In fact the new ImageRaster does nearly support that already as it has ImageCodec classes which know how to do the encoding/decoding of the various image formats - so all it would need is a new constructor for ImageRaster which allowed the codec to be specified rather than looking it up from the pre-supplied ones. People could then write their own codecs at will.
Obviously that would be harder in the old enum-based approach…although I’m not convinced of the utility as the only image formats supported are those in that enum anyway (since there is no way to specify that the image is in that format anyway)…so if a format is not in the enum then you cannot create an Image in that format so its pointless being able to encode/decode in that format…
One possibility would be to ditch the Format enum and have it be a base class that knows its codec and any other information currently used. People could then implement their own Formats at any point simply by creating their own implementation of the base class and hence create any type of Image.
Again I’m not sure that flexibility would ever actually be useful though?
@zarch said:
Yes, the static factory pattern would make sense in this case, particularly with breaking it out into ImageRaster instead of leveraging the enum.
In fact the new ImageRaster does nearly support that already as it has ImageCodec classes which know how to do the encoding/decoding of the various image formats - so all it would need is a new constructor for ImageRaster which allowed the codec to be specified rather than looking it up from the pre-supplied ones. People could then write their own codecs at will.
Obviously that would be harder in the old enum-based approach...although I'm not convinced of the utility as the only image formats supported are those in that enum anyway (since there is no way to specify that the image is in that format anyway)...so if a format is not in the enum then you cannot create an Image in that format so its pointless being able to encode/decode in that format...
One possibility would be to ditch the Format enum and have it be a base class that knows its codec and any other information currently used. People could then implement their own Formats at any point simply by creating their own implementation of the base class and hence create any type of Image.
Again I'm not sure that flexibility would ever actually be useful though?
Any time one ties one part of an engine needlessly to another I think it ends up in regret. The idea of a read/writable Raster has very little to do with JME Image and in my opinion shouldn't be tied directly to it. The fact that there is an Image-based Raster is one particular implementation... but someone could also write a Raster adapter for Java's BufferedImage or any number of other use-cases we are not prescient enough to see right now. If you write code like an ImagePainter or an SVG renderer or whatever that writes to Raster then you can use it regardless of the underlying implementation.
Furthermore, if you want to experiment with different format specific implementations/optimizations then there are fewer things to mess with. Potentially, we reach a point where adding a new core format can be done simply by adding a new enum + raster implementation.
There are no down sides. There are many potential up sides.
This is a separate issue from the format enum to me.
Well generally the jme3 core has some points where it is somewhat tightly coupled to its own limitations. Which is fine mostly for the moment, but of course having more open interfaces would be great.
Much as I’d like to let this thread die I’d like to know if anything further is going to happen. In particular the performance penalty of the new way of doing things makes me reluctant to move over to it.
So is it dead, set in stone, that’s the way things will always be or is anything going to change?
Kirill is looking into the speed difference and pauls suggestion, yes.
Ok, in that case I’ll leave it as is for now and wait to see what the final situation is before I do anything. 
I guess we will keep something very similar to the current version structurally though, the speed difference is most probably an implementation detail that can be overcome.
I agree that ImageRaster or not is unlikely to be making any significant speed difference. It’s going to be the implementation of the get/set.
My post earlier had a description of why I expected there to be a performance difference:
With the original implementation getPixel on RGB would immediately delegate to the enumeration and call the relevant method. That would read the data in the correct format – even taking advantage of things like subsequent getByte being able to read the correct values without needing to seek around. This is both much easier for the compiler to optimise as the call stack is simpler and there is no branching and no map look ups and directly faster due to avoiding any unneeded seeks or bitwise operations. Without running performance tests I’ve no idea whether the difference is 1% or 400% (either is entirely possible) but the only thing I can be sure of is that the new approach will be at least a little slower.
I might do some performance tests later as I’m going to need to move ImagePainter over to be built on top of ImageRaster assuming performance is acceptable.
Take a simple example of RGBA – in that case you can literally do getByte 4 times, dump the value into a ColorRGBA and you are done. All the other byte-per-channel formats are also nearly as simple. In the new implementation though I’ve not looked in detail but it seems to read them into a local store, move them around, bitmask them, etc because the complexity needed for the other formats is still there even in this simple case.
It's noticeable that in the figures from my performance test the largest difference was RGBA8 to RGBA8 (120% slowdown) - whereas BGR8 which I hadn't optimised getPixel for the difference is only 3%. That implies that the seek process is much slower than sequential reads although I've not done any real digging to verify so it could be caused by something else entirely.
@zarch
Your version is a modification of the actual image class, right? Can’t that be a raster as well, containing an image object? I guess most of the jME users doesn’t want/need to be able to modify their images. Those of us who want can just use your optimized getpixel stuff, and the image tools, and you could just continue to do whatever you want with it.
@androlo said:
@zarch
Your version is a modification of the actual image class, right? Can't that be a raster as well, containing an image object? I guess most of the jME users doesn't want/need to be able to modify their images. Those of us who want can just use your optimized getpixel stuff, and the image tools, and you could just continue to do whatever you want with it.
If the new implementation adopts my interface ideas then it can have more optimal implementations for the cases zarch cites where the mapping is straight forward. The current concrete class is limiting in this respect.
Edit: note: if my local jme checkout were more current then I'd just do the interface thing myself because it's a harmless change functionally. I may update later today and see. I think momoko_fan is on a trip.
1 Like
@androlo said:
@zarch
Your version is a modification of the actual image class, right? Can't that be a raster as well, containing an image object? I guess most of the jME users doesn't want/need to be able to modify their images. Those of us who want can just use your optimized getpixel stuff, and the image tools, and you could just continue to do whatever you want with it.
Well, most of the functionality in mine is actually on the Format enum so it doesn't add any weight to Image objects at all (as enums are singletons) - there were a few convenience functions on Image but that was it.
It would have been straightforward to move it from the enum into an enummap and create an ImageRaster for it but I don't really want to cause a fork and end up with two different systems doing the same thing. So long as there is no large performance overhead I don't really care how it works behind the scenes so I'm going to wait and see what happens with the "official" version. If performance is still too low then I'll probably have to put the encode/decode logic for the optimised solutions into ImagePainter and then fall back to ImageRaster for anything else.
@pspeed said:
If the new implementation adopts my interface ideas then it can have more optimal implementations for the cases zarch cites where the mapping is straight forward. The current concrete class is limiting in this respect.
Edit: note: if my local jme checkout were more current then I'd just do the interface thing myself because it's a harmless change functionally. I may update later today and see. I think momoko_fan is on a trip.
Essentially all the single byte->channel mappings (BGR8, RGB8, etc) can be done very efficiently by specific implementations although of course the RGB ones are the super-simplest case. As soon as byte fiddling etc is required then I'd expect the performance gains to be much smaller from the specific implementation over the generic ones although even then there is potential for savings.
I see setpixel is hooked up to the float->half converter as well. That is great. Gonna try using 16 bit format for heightmaps.
This class is very good. Thanks to all contributors.
Is @zarch’s ImagePainter available for the jme3 ImageRaster implementation?