Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Vt] Add iterator construction for arrays. #644

Merged
merged 2 commits into from
May 16, 2019

Conversation

superfunc
Copy link
Contributor

Discussed this one offline with @gitamohr , seems like a nice addition. Its useful
for us when constructing VtArrays from foreign data sources (Maya APIs, etc) which
return T*s or vector<T>s.

Description of Change(s)

VtArray mimics much of std::vector's interface. This adds
iterator construction, which had been previously omitted.
In the past, VtArray supported dimensions, which made such
construction unwieldy; it is now a flat container, making
this a viable option.

Fixes Issue(s)

  • None filed

@jtran56
Copy link

jtran56 commented Oct 9, 2018

Filed as internal issue #USD-4809.

@c64kernal
Copy link
Contributor

Hey @superfunc here's the feedback for this one:

  • The constructor needs to be properly constrained so it's not ambiguous with the (size_t, value_type)
    constructor when value_type is integral. It should behave the same as
    the std::vector ctors.

  • It should accept "LegacyInputIterators" not just ForwardIterators.

  • It should be written to avoid filling the array with default-initialized values only to destroy and overwrite them immediately.

Thanks!

@c64kernal c64kernal added the blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed label Jan 14, 2019
@superfunc
Copy link
Contributor Author

Hey @c64kernal for the third note, I may just not be seeing it, as far as I can tell its the same way other constructors are doing this, e.g.

289     /// Initialize array from the contents of a \p initializerList.                                                     
290     VtArray(std::initializer_list<ELEM> initializerList)                                                                
291         : VtArray() {                                                                                                   
292         assign(initializerList);                                                                                        
293     }

Its calling into the VtArray() constructor which just does _data(nullptr) as far as I can tell.

@c64kernal
Copy link
Contributor

Hey @superfunc -- no worries about the 3rd point, I'll file an internal bug to address that, as you point out, the rest of the constructors have the same issue.

VtArray mimics much of std::vector's interface. This adds
iterator construction, which had been previously omitted.
In the past, VtArray supported dimensions, which made such
construction unwieldy; it is now a flat container, making
this a viable option.
@superfunc
Copy link
Contributor Author

Ok, I think that should fix it, @c64kernal . Sorry the template constraint is so ugly, didn't know another way to do it haha.

@c64kernal c64kernal removed the blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed label Apr 19, 2019
@c64kernal
Copy link
Contributor

Hi @superfunc -- apologies on the delay on this one, but I had a hard time getting it to compile in our environment and needed to find time to figure it out. I fixed the is_same<size_t,...> check with is_integral<...> because is_same<size_t,...> doesn't quite catch all the cases. Also had to change a use of auto in the test that was breaking on strict builds.

Thanks again for PR, should be merged on the next push.

@superfunc
Copy link
Contributor Author

Thanks for poking at this, sorry I've been too swamped to clean it up properly.

@pixar-oss pixar-oss merged commit ea08a26 into PixarAnimationStudios:dev May 16, 2019
pixar-oss added a commit that referenced this pull request May 16, 2019
[Vt] Add iterator construction for arrays.

(Internal change: 1960730)
AdamFelt pushed a commit to autodesk-forks/USD that referenced this pull request Apr 16, 2024
…ationStudios#644)

* needs review.

* Disabling color correction for now.

* Provided Vulakn 1.0 support and enabled Alpha to coverage

* Fixed few vulkan validation errors, fixed index binding offset issue, fixed bad load operation flag applied to subsequent draw batches belonging to the same render task.

* Code clean up

* Undid some andoird changes kaiquan pushed since they should be focused to the Android branch. Some code clean up, providing support to disable MSAA on Vulkan

* Fixed the build/run issues of HgiVulkan on Linux

(cherry picked from commit 37eab849f16cb2d4a9e672c402aa1e6fec42193c)

* fixing bad cmake indentation indroduced during vulkan linux stability in build script

* Cherry picking some code review changes from the resolve branch

* Undoing code review changes for accuracy

(cherry picked from commit 4992590f94cb167d9ceaca627e1c5e015f8ee760)

* merge fixes and bug fixes.

* optimizing implementation by removing unnecessary code.

* Code review fixes.

* providing fix for correct render target clear on Vulkan without breaking OpenGL

* cherry picking fix that applies to Vulkan for correct clear render target huristic without breaking opengl

* vode cleanup + removing MSAA to test openGL stability

* code review fixes.

* fixing compilation on Android

* fixing some bad code for Vulkan path.

* fix for failing gl clip distance unit test

* bad merge fix.

Co-authored-by: kapoorv <vipul.kapoor@msn.com>
Co-authored-by: Andy Shiue <andy.shiue@moiggi.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants