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

X3d 3d ssfile #1086

Merged
merged 95 commits into from
Jul 9, 2021
Merged

X3d 3d ssfile #1086

merged 95 commits into from
Jul 9, 2021

Conversation

kgbudge
Copy link
Contributor

@kgbudge kgbudge commented Jul 9, 2021

Background

  • We have a use case for transport run on a pure hexahedral X3D mesh.
  • While adding the necessary capability to our code library, we found that the X3D_Draco_Mesh_Reader sorts the nodes on a boundary face by node index, destroying the original counterclockwise ordering of the nodes.

Purpose of Pull Request

  • Modify X3D_Draco_Mesh_Reader to store boundary face nodes in the original counterclockwise node ordering, rather than sorted by index.

Description of changes

  • The reader sorts the nodes by node index to allow easy matching of boundary faces to cell faces. This is fine, but the code then added the sorted version to the side to node linkage. The code has been modified to add the original version (listed in counterclockwise order) to the side to node linkage instead.

Status

kgbudge added 30 commits July 30, 2020 13:59
@RyanWollaeger
Copy link
Contributor

@kgbudge It looks like the diff is picking up changes from previous PRs. @KineticTheory do you know what's going one here? Does Github get confused from certain rebase/merges (I see a lot of merge commits in the commit log for this PR)?

@KineticTheory
Copy link
Collaborator

@kgbudge It looks like the diff is picking up changes from previous PRs. @KineticTheory do you know what's going one here? Does Github get confused from certain rebase/merges (I see a lot of merge commits in the commit log for this PR)?

Long lived git branches often cause this issue -- especially when there are so many merge commits from upstream/develop. @kgbudge Can you apply this change set to a new branch based on the current upstream/develop and update the PR?

@KineticTheory
Copy link
Collaborator

@kgbudge It looks like the diff is picking up changes from previous PRs. @KineticTheory do you know what's going one here? Does Github get confused from certain rebase/merges (I see a lot of merge commits in the commit log for this PR)?

Long lived git branches often cause this issue -- especially when there are so many merge commits from upstream/develop. @kgbudge Can you apply this change set to a new branch based on the current upstream/develop and update the PR?

The diff is clean now. The changes look good to me.

@RyanWollaeger Can you review these changes?

Copy link
Contributor

@RyanWollaeger RyanWollaeger left a comment

Choose a reason for hiding this comment

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

LGTM

@KineticTheory
Copy link
Collaborator

I'll merge this if the CI testing passes.

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #1086 (6f15349) into develop (dcb5c56) will not change coverage.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           develop   #1086   +/-   ##
=======================================
  Coverage     88.7%   88.7%           
=======================================
  Files          374     374           
  Lines        18601   18601           
=======================================
  Hits         16505   16505           
  Misses        2096    2096           

@KineticTheory KineticTheory merged commit 9859953 into lanl:develop Jul 9, 2021
@kgbudge kgbudge deleted the x3d_3d_ssfile branch July 26, 2021 14:30
@kgbudge kgbudge restored the x3d_3d_ssfile branch July 26, 2021 14:32
@kgbudge kgbudge deleted the x3d_3d_ssfile branch July 26, 2021 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants