-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
deprecate changing buffer size with setArray() #17418
Conversation
I think As mentioned here #17242 (comment), we should encourage developers to call
Like mentioned here #14730 (comment), I vote to remove |
I'm not sure if anyone uses it in this way, but using setArray() to replace a buffer's data with an new equal sized buffer gives the user a method to use the purportedly superior BufferSubData() rather than BufferData() which using a new BufferAttribute() requires. It could be argued that this is a reason to support setArray(), say for getting new ArrayBuffers from a worker. Anyway, just deprecating resizing is the minimum change required to sort out the dynamic flag. Regarding .dispose() I completely agree it should be the preferred method and should be added, but with WeakMap() in use it isn't essential. Regarding the naming of .dynamic I haven't got an opinion either way, |
I agree with @Mugen87, I think we should just deprecate |
The only thing that annoys me is suggesting using <rant> A method named We could then follow |
OK, no problem with that. I don't use setArray() myself, but playing devil's advocate to some extent for anyone that does, after the pain with it previously. Do you want a PR to deprecate it or does @Mugen87 have it in progress? Regards having setAttribute, if serious, you would want to do that before removing render support for Geometry() I assume. |
First step: #17439 |
Would |
@aardgoose Would you like to do a PR which deprecates In this change, it would be also necessary to update the docs and unit tests. |
It is more accurate, but... as a user, I imagine users will actually want to do like this instead:
To my best understanding, this would require |
@EliasHasle Can you please clarify what you are talking about? The implementation of |
Renaming the methods would not be a breaking change since you can keep the "old" methods in |
OK. That would be possible with my suggestion too. Anyway, I mentioned it because there was talk of a possible API change for this (whether breaking or not). |
Well, you would need a polyfill for IE11 since it does not support the Proxy object. |
I don't think Proxies can be (fully) polyfilled at all. In particular, I see no way except proxies to trap arbitrary keys (attribute names in my example). Off-topic about IE11 supportAnyway, why support IE11, which is used by less than 2%, has not seen a release since 2013 and is abandoned in favor of Edge by Microsoft. Not to mention it is a horrible browser, and I can never imagine that those interested in three.js or other WebGL applications ever use it. |
This thread is not the right place to discuss IE11 support. It's just the status quo and the development needs to respect the current supported browsers. |
PR to deprecate setArray #17454 |
replaced with #17454 |
Fixes #69. This works around an issue introduced in Three r117 where a given geometry uses the size of any InstancedBufferAttribute as a max for `instanceCount`, and caches that for the lifetime of the geometry even if the attribute is replaced with one of a different size. It's unclear if this is a ThreeJS bug or a if our approach of "resizing" by replacing the attribute is truly unsupported; the discussion in mrdoob/three.js#19706 is ambiguous, mrdoob/three.js#19595 implies it's not allowed, and mrdoob/three.js#17418 implies it should be allowed.
Relating to #14730 issues with BufferAttribute.dynamic flag overloading.
Deprecate changing a BufferAttribute's size with .setArray() as a prelude to removing this ability.
Now all internal renderer and GPU state relating to BufferAttributes is weakly referenced with the use of a WeakMap, an attrribute can simply be resized by overwriting with a new BufferAttribute with Geometry.addAttribute() without long term memory leaks, this has little additional overhead over using setArray() to resize an attribute. #17063 would allow users explict control over heap usage if waiting for GC , but is not a strict requirement.
After one or more releases this warning can be replaced with an exception, and WebGLBufferATtribute simplified to restore the dynamic flag to it's original purpose.