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

[Proposal]: Change return types of methods that return List<> to IEnumerable<> #8

Open
MechWarrior99 opened this issue Jul 11, 2021 · 1 comment

Comments

@MechWarrior99
Copy link
Contributor

MechWarrior99 commented Jul 11, 2021

Summary

Change the return type of methods that return List<> to IEnumerable<>.

Motivation

Manipulating the lists that returns from the methods have no effect on the actual BMesh, as such there is really no reason you ever should or would need to add/remove items to the List<>s. It also does not clearly communicate the purpose and use of the returned collection when it is a List<>.
Where as IEnumerable<> more clearly communicates the intent behind the method.
Returning a List<> does not follow the C# design guidelines. An added benefit of returning IEnumerable<> is that the methods can make uses of yield return for potentially slightly better performance.

Drawbacks

This is a breaking changing if people use any of the List<> methods such as Add() Remove() or Contains(), or for people that declare the variable type when assigning the returned value List<Edge> edges = vertex.NeighborEdges().

Alternatives

One option is to return IReadOnlyList<> instead so people still have access to the indexer and Count property. However this makes it less generic and can no longer use yield return.

I would be happy to make a PR for this change if desired after any further discussion.

@eliemichel
Copy link
Owner

eliemichel commented Jul 11, 2021

Agreed as well!


Since we are on the way to breaking changes, it is also time to think about performances a bit. I really wanted to foster ease of use first, but it would be worth thinking about for instance limiting new memory allocation. For instance:

v1.attributes["uv"] = new FloatAttributeValue(0.0f, 0.0f);

is less efficient than

var uv = v1.attributes["uv"].asFloat();
uv.data[0] = 0.0f;
uv.data[1] = 0.0f;

but the latter is a bit annoying to write.

(edit: see #11)

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

No branches or pull requests

2 participants