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

Added methods to expose the unmanaged pixel array. #138

Closed
wants to merge 1 commit into from
Closed

Added methods to expose the unmanaged pixel array. #138

wants to merge 1 commit into from

Conversation

Gitii
Copy link

@Gitii Gitii commented Dec 3, 2016

Hi,

I needed some methods to access the image data directly (pointer).
And to construct an image from native memory.

I would like to integrate my changes back into the original code base.

Thanks

Added new constructor which takes an IntPtr+Length
@LaurentGomila
Copy link
Member

Hi

It is not enough to say "I needed it so it should be in SFML", you should explain why you think your modification deserves to be integrated into SFML.Net. Don't the existing methods (using managed arrays) already allow to do what you want?

@Gitii
Copy link
Author

Gitii commented Dec 3, 2016

The main reason because I needed these methods it, that I wanted to reduce marshalling between managed and native memory. In my use case, I had the images in native memory. Instead of copying it into a managed array (and some time later back into the native memory), I used these methods to directly use the image data with sfml (net). I wanted to reduce copying and the gc pressure. I admit, it's an edge case. In addition I wanted to directly access the pixel data without copying it into an managed array first (e.g. to pass it 'directly' to the graphics driver which would create a copy on its own).

@LaurentGomila
Copy link
Member

What kind of application are you developing ? And does it make a significant difference, without these managed <-> native copies?

@Gitii
Copy link
Author

Gitii commented Dec 3, 2016

I have never done any measuring (I just counted the allocations and choosed this approach (to modify sfml.net)). I would guess, the difference in performance depends on the number of copies and the size of the involved data. And how the gc handles the memory allocations.
I am using sfml (net) in one of my game development 'playground' projects. Nothing serious. I just adopted sfml image to my workflow (resource management).

@LaurentGomila
Copy link
Member

So, this change is about an unverified optimization in an edge case context? Doesn't sound like a good candidate for integration... 😛

@Gitii
Copy link
Author

Gitii commented Dec 4, 2016

You are right. It's unverified and an edge case. My changes does not introduce new functionality (compared to the original sfml) but another way to access&use the pixel data.
You can close this pull request, if you think, it does not 'belong' to sfml.net.

@LaurentGomila
Copy link
Member

LaurentGomila commented Dec 4, 2016

It could belong to SFML, but with better arguments. So yes, I'll close this for now.

Thanks for the contribution anyway.

@dabbertorres
Copy link
Contributor

This would be an interesting (and potentially useful) addition I think.

As for other arguments, what about ease use of with other libraries? For instance, let's say I'd like to use a native library for some sort of image analysis. Now, I'd have to write the P/Invoke functions, and then I would additionally need to write wrapper functions around those, to copy the array data to and from native and managed.
If SFML.NET provided functionality such as Gitii provided, I wouldn't necessarily need to write the wrapper functions.

It could then be done (more or less) as simple as:
var edges = ImageAnalysis.Edges(sfmlImage.NativeData, sfmlImage.Size.X * sfmlImage.Size.Y * 4);

Additionally, the C++ and C APIs already provide direct access to the data via pointer. SFML.NET is (currently) artificially limiting its functionality relative to those.

A counter argument would be API clutter, due to the current Pixels property already existing.

Thoughts? (Should this be moved to the forums?)

@LaurentGomila
Copy link
Member

LaurentGomila commented Dec 5, 2016

potentially
I think
it would

If you want to find arguments to support a given feature, you can always come up with potential ideas. For SFML, I like to discuss about concrete arguments from real use cases. It's not that I find this addition useless, but exposing unmanaged pointers in a SFML.Net API is not clean, and it doesn't allow anything that is not already possible -- it just saves CPU cycles and a few lines of code -- so why add it if nobody actually needs it?

For instance, let's say I'd like to use a native library for some sort of image analysis. Now, I'd have to write the P/Invoke functions, and then I would additionally need to write wrapper functions around those, to copy the array data to and from native and managed.

I think people who deal with 3rd party libraries that need unmanaged stuff, are used to managed <-> unmanaged conversions, and don't mind the extra steps required for the conversion.

Additionally, the C++ and C APIs already provide direct access to the data via pointer. SFML.NET is (currently) artificially limiting its functionality relative to those.

C and C++ expose a direct pointer because that's how you provide access to an array in these languages. SFML.Net is a different language with a different way of managing memory, and the goal in this binding is to stick to the language as close as possible, making the library look like any other (managed) library, and not a "hackish API around a C library".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants