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 remove silently for UpdateList #23

Closed
wants to merge 2 commits into from

Conversation

FancySensei
Copy link
Contributor

If you are confident with your code you can now remove your update functions from the list without any throws.

@kkjamie
Copy link
Contributor

kkjamie commented May 8, 2019

Just to provide a bit of background on this.

The regular Remove function will throw if you try to remove a function that wasn't in the list, we want that because if you run into this situation it's symptomatic of some incorrect logic in your code, and we want you to know so you can go fix this. Without this we maybe hiding bugs.

On the other hand checking this is slower, given the performance critical context (it's an update list, it does a list of things every frame), it's nice to have a function that is highly optimized.

My concerns for this are how do we indicate which function you should use. Many people may get the exceptions and instead of hunting down the problem, they see RemoveSilently and go with that one.

I'm also conflicted on the whole update list being public, as documented #22. It's from the duck-tween
library rather than duck-utils part of me thinks it's only use case should be within this library, and we can make it internal. It never feels right "Borrowing" a util from a library you have used for another purpose. I wouldn't be opposed to adding a type of update list into duck-utils for public use

@kkjamie
Copy link
Contributor

kkjamie commented May 8, 2019

it maybe more beneficial to keep the API as a single function, and use scripting defines. So in regular dev environment you get the check and the throw, but in production, you get maximum performance and no chance to throw and crash the application.

That way there is no ambiguity between API and no temptation to use the "silent" one to lazily get rid of your errors.

@FancySensei
Copy link
Contributor Author

Yeah I agree the UpdateList deserves a separated space rather than hiding in the mighty tween umbrella.

I don't think the compiler flag would work, however. It's too hard to pre-define some flags for all those random projects outside the world.

I think it's the users' responsibility to do things like:

#if PRODUCTION
    updateList.RemoveSilently(blah);
#else
    updateList.Remove(blah);
#endif

@FenrirWillow
Copy link

Instead of RemoveSilently, perhaps this function can be named RemoveUnsafe. Several APIs in Unreal for example use this notation. There is also a hidden contract that Unsafe usually means you are skipping some step (locking, bounds checks etc.). Don't know how you feel about it @kkjamie in terms of general Duck context.

@kkjamie
Copy link
Contributor

kkjamie commented May 9, 2019

@FenrirWillow I agree.

However in this case as discussed previously I think we should make this particular utility internal to this library, making chuan's changes redundant. I think we can create a similar thing for public use with this kind of API seperately, and it can go into duck-util.

@kkjamie kkjamie closed this May 9, 2019
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