Skip to content
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

BitmapData.threshold() for CPP target. #72

Merged
merged 10 commits into from Apr 9, 2013

Conversation

Projects
None yet
3 participants
@larsiusprime
Copy link
Contributor

commented Apr 8, 2013

BitmapData.threshold() implementation for cpp target. Pretty fast! Also tested and accurate - same visuals and return values as Flash API version. Got lots of help from crazysam (Sam Batista) and the HaxeFlixel community on this one.

This is my first pull request, so let me know if I did it wrong or need to fix something!

More details:
http://www.haxeflixel.com/forum/general-discussion/bitmapdatathreshold-palette-swap-solution-cpp-target
http://www.nme.io/community/forums/feature-requests/my-implementation-bitmapdatathreshold-cpp-target/#ccm-discussion-post-anchor

Three test images are used below: one grid of greyscale values, one grid of bluescale values, and one giant huge benchmark image full of yellow smiley faces (the benchmark color-swaps the yellow shades to cyan).

nme_threshold_flash
nme_threshold_cpp

Lars Doucet added some commits Apr 4, 2013

@gamedevsam

This comment has been minimized.

Copy link

commented on native/display/BitmapData.hx in 32f1866 Apr 6, 2013

This should just be an Int for Haxe 3 compatibility.

@gamedevsam

This comment has been minimized.

Copy link

commented on native/display/BitmapData.hx in 32f1866 Apr 6, 2013

Probably worth adding a comment above explaining what this does (could just copy a trimmed version from the AS3 docs). The important part is to mention which operation strings are valid: "One of the following comparison operators, passed as a String: "<", "<=", ">", ">=", "==", "!=""

Lars Doucet added some commits Apr 7, 2013

Lars Doucet
cpp BitmapData.threshold() now passes these tests:
-Basic functionality
-Decently optimized for straight "==" palette swapping
-Returns same # of pixels changed as flash target
-Returns same visual output unless you mess with destPoint
(will figure out destPoint later)
@andyli

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2013

Thanks for the pull request! I like how you've compared the results between flash and cpp :)

However please also make sure that it is haxe3 compatible. As crazysam and the TravisCI build result've suggested, it is that Int32 has to be changed.

And by the way it is a good practice to Memory.select(null); at the end of a memory process, such that there wouldn't be an extra reference hold that ByteArray.

Also please remove that command-line.n.

You can make the changes and push to your fork and this pull request will be updated.
For next time, remember to create a separate topic branch to implement the changes, which will save yourself some effort to sync between the upstream and your fork. See: https://help.github.com/articles/using-pull-requests

@larsiusprime

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2013

Thanks! I'm new to this, so I'll make those changes now.

Lars Doucet added some commits Apr 8, 2013

Lars Doucet
threshold(): Changed Int32 -> Int for Haxe 3 compatibility; Added Mem…
…ory.select(null) at end of function calls.
@larsiusprime

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2013

Okay, I think that's everything then? Let me know if I need to do something else.

@andyli

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2013

Sorry but there are a few references to Int32 left. Please remove them and check if the result is still actuate.

@larsiusprime

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2013

Okay... so that makes things complicated. The remaining references to Int32 are for the Int32.ucompare() function, which is pretty central to this implementation. Without ucompare(), the operator comparisons simply don't work - "<" and ">" don't return the right results, which in turn screws up "<=" and ">=".

Is there a Haxe3 alternative to Int32.ucompare() ?

@andyli

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2013

You may try extracting the ucompare function from haxe 2.10 source which I believe is still correct and functional for haxe 3.

@larsiusprime

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2013

Okay, I'll try that!

One question - where's a reasonable place to put it? I could just keep it around locally in BitmapData.hx as an inline utility function or something, but would it be better to put it in, say, Math?

Lars Doucet added some commits Apr 9, 2013

Lars Doucet
Added RealyUniqueName's Int32.ucompare() replacement. Not getting con…
…sistent visual results with it, but the logic seems right, still working...
Lars Doucet
@larsiusprime

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2013

Okay, I think I got it!

Tests with the new ucompare function are below.

I believe this removes the Int32 dependencies. I now have a local ucompare function in BitmapData.hx - this is RealyUniqueName's contribution. I figure this should probably eventually go in some utility class in the Std library - would Math be appropriate? Until someone gives me a better idea I'll just leave it here for now so I don't mess anything up.

I also have a repo for a simple test suite for this function (it generates the test images you see below) in case someone else wants to check my work:
https://github.com/larsiusprime/nme-benchmark-threshold

nme_threshold_flash_2
nme_threshold_cpp_2

@andyli

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2013

Very cool! Thanks for everyone's effort!

andyli added a commit that referenced this pull request Apr 9, 2013

Merge pull request #72 from larsiusprime/master
BitmapData.threshold() for CPP target.

@andyli andyli merged commit 8be687e into haxenme:master Apr 9, 2013

1 check passed

default The Travis build passed
Details
@larsiusprime

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2013

Yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.