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

DYN-6842 Convert Helix instancing feature flag to a Preference Setting #14675

Merged
merged 10 commits into from
Apr 11, 2024

Conversation

saintentropy
Copy link
Contributor

@saintentropy saintentropy commented Dec 3, 2023

Purpose

Convert the Helix Instancing feature flag to a Preference Setting.

image

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

…ancingOption

# Conflicts:
#	src/DynamoCoreWpf/Properties/Resources.en-US.resx
#	src/DynamoCoreWpf/Properties/Resources.resx
@@ -1971,8 +1970,7 @@ internal virtual void AggregateRenderPackages(IEnumerable<HelixRenderPackage> pa
//for each instancable item and add instance transforms.
//If we have any mesh geometry that was not associated with an instance, remove the previously added
//mesh data from the render package so the remaining mesh can be added to the scene.
if (rp.MeshVertexRangesAssociatedWithInstancing.Any()
&& DynamoModel.FeatureFlags?.CheckFeatureFlag<bool>("graphics-primitive-instancing", false) == true)
if (rp.MeshVertexRangesAssociatedWithInstancing.Any())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't you first checking if the useinstancing flag is on over here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yep

Copy link
Contributor Author

@saintentropy saintentropy Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @aparajit-pratap this one is back.... So I think there is a question to be looked at here. In general we have fed preference level items that impact how something is tessellated in the TessellationParameters. Show Edges for example. We go Preferences -> TessellationParameters -> Tesselate -> Render. Render doesn't know about TessellationParameters. If we do the same here then the Render function should not be deciding to use Instancing... It should always use instancing if instancing data is present. The way the data in the RenderPackage works it actually might render everything wrong if it ignores the data associated with instancing since all the vertices are stored in the same array.

@QilongTang QilongTang added this to the 3.0 milestone Dec 5, 2023
# Conflicts:
#	src/DynamoCoreWpf/Properties/Resources.en-US.resx
#	src/DynamoCoreWpf/Properties/Resources.resx
Copy link

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

Copy link

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

Copy link

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

Copy link

github-actions bot commented Dec 12, 2023

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@QilongTang
Copy link
Contributor

@saintentropy @aparajit-pratap Not sure what's the plan for this, I will move this to 3.1 milestone so we can close the 3.0 milestone on Github to wrap up the release.

@QilongTang QilongTang modified the milestones: 3.0, 3.1 Jan 3, 2024
@sm6srw sm6srw marked this pull request as draft January 16, 2024 18:57
@QilongTang
Copy link
Contributor

Hi @saintentropy @aparajit-pratap Please confirm this PR's state and Jira task?

@saintentropy saintentropy changed the title Convert Helix instancing feature flag to a Preference Setting DYN-6842 Convert Helix instancing feature flag to a Preference Setting Apr 5, 2024
@reddyashish reddyashish marked this pull request as ready for review April 11, 2024 03:09
Copy link
Contributor

@reddyashish reddyashish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. Might have to update some tests, let's wait for self-service.

@reddyashish reddyashish merged commit bfd2408 into DynamoDS:master Apr 11, 2024
23 checks passed
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.

4 participants