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

VertexArray indexer doesnt returns a reference #195

Open
jcbritobr opened this issue Oct 31, 2020 · 6 comments
Open

VertexArray indexer doesnt returns a reference #195

jcbritobr opened this issue Oct 31, 2020 · 6 comments

Comments

@jcbritobr
Copy link

jcbritobr commented Oct 31, 2020

This indexer in VertexArray does'nt returns a reference to a vertex. Docs are wrong, or the indexer is wrong. A deepy copy leads to poor performance in painting.

//
        // Summary:
        //     Read-write access to vertices by their index. This function doesn't check index,
        //     it must be in range [0, VertexCount - 1]. The behaviour is undefined otherwise.
        //
        // Parameters:
        //   index:
        //     Index of the vertex to get
        //
        // Returns:
        //     Reference to the index-th vertex
        public Vertex this[uint index] { get; set; }

This code will not work because v is a Vertex copy.

        Vertex v = array[(uint)i];
        v.Color = new Color((byte)r.Next(0, 255), (byte)r.Next(0, 255), (byte)r.Next(0, 255));
        v.Position = new Vector2f(r.Next(0, 800), r.Next(0, 600));
@Rosst0pher
Copy link
Contributor

I'm gonna assume the docs are wrong on this detail as they look copied from the C++ documentation where this indexer does return a reference. I'd also imagine the VertexArray class was originally written before return-by-reference for value types was introduced into C#.

@jcbritobr
Copy link
Author

I'm gonna assume the docs are wrong on this detail as they look copied from the C++ documentation where this indexer does return a reference. I'd also imagine the VertexArray class was originally written before return-by-reference for value types was introduced into C#.

Yep. My sugestion is to maintain indexer as it is, because ref can't work with this operator and create a

public ref Vertex GetVertex(int index) {
    ...
}

This will boost the vertex painting performance a lot.

@Rosst0pher
Copy link
Contributor

In this case, I don't believe the Vertex copy directly affects draw performance of a VertexArray as this indexer isn't used when drawing, but it obviously contributes to the time it takes to update individual vertexs. Any change for performance boosts is something I'd encorage benchmarking first.

Regarding your suggestion, I'd ask @DemoXinMC for input.

@jcbritobr
Copy link
Author

Yes, I'm refer to the vertex copy time. It seems an implementation that can be tested easily.

@jcbritobr
Copy link
Author

jcbritobr commented Nov 1, 2020

I discover this using the same code in c#, pascal and rust. Pascal and Rust bindings already read a reference from vertex array. Only c# doesn't. Pascal and Rust are able to update a 1 Mi vertex in this way(pascal = 24fps, rust=42fps c#=16fps). C# got stucked to update a vertex array of this size. If you need I have the three languages implementations for the same algorithms.

@eXpl0it3r eXpl0it3r added the Bug label Jun 15, 2023
@eXpl0it3r
Copy link
Member

Since we're doing pInvoke calls, we can't really provide a reference.

Anyone willing to update the documentation?

@eXpl0it3r eXpl0it3r added the Docs label Jun 15, 2023
DemoXinMC added a commit to DemoXinMC/SFML.Net that referenced this issue Jan 3, 2024
* Addresses requests in SFML#194 and SFML#195
* Updated all [Obsolete] attributes to be less repetitive
* Added a large number of quick reference links to existing summaries
* Corrected various typos and unparsable markup(\a and \p)
* Added additional formatting to numerous summaries, resulting in more readable tooltips
* Added a suggestion to use TimeSpan over SFML's Time while working within a managed environment
DemoXinMC added a commit to DemoXinMC/SFML.Net that referenced this issue Jan 4, 2024
* Addresses requests in SFML#194 and SFML#195
* Updated all [Obsolete] attributes to be less repetitive
* Added a large number of quick reference links to existing summaries
* Corrected various typos and unparsable markup(\a and \p)
* Added additional formatting to numerous summaries, resulting in more readable tooltips
* Added a suggestion to use TimeSpan over SFML's Time while working within a managed environment
* Added a warning to `VertexBuffer.NativeHandle`'s getter informing the end-user that most people shouldn't need that property and to be cautious
DemoXinMC added a commit to DemoXinMC/SFML.Net that referenced this issue Feb 17, 2024
* Addresses requests in SFML#194 and SFML#195
* Updated all [Obsolete] attributes to be less repetitive
* Added a large number of quick reference links to existing summaries
* Corrected various typos and unparsable markup(\a and \p)
* Added additional formatting to numerous summaries, resulting in more readable tooltips
* Added a suggestion to use TimeSpan over SFML's Time while working within a managed environment
eXpl0it3r pushed a commit that referenced this issue Feb 17, 2024
* Addresses requests in #194 and #195
* Updated all [Obsolete] attributes to be less repetitive
* Added a large number of quick reference links to existing summaries
* Corrected various typos and unparsable markup(\a and \p)
* Added additional formatting to numerous summaries, resulting in more readable tooltips
* Added a suggestion to use TimeSpan over SFML's Time while working within a managed environment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants