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

Fixed a compilation issue in generateGridVertices. #47

Closed
wants to merge 1 commit into from

Conversation

zhivkob
Copy link
Contributor

@zhivkob zhivkob commented Oct 26, 2021

After changes from pull request #45 initialization with a grid no longer worked because the file didn't compile.

After changes from pull request artem-ogre#45 initialization with a grid no longer worked and the file didn't compile at all.
@artem-ogre
Copy link
Owner

artem-ogre commented Oct 26, 2021

@zhivkob Thank you for reporting the problem and fixing it.
I've done the fixes based on your work here: https://github.com/artem-ogre/CDT/tree/bugfix/fix-initialize-with-grid

Could you please replace your commit with the commit from that branch 7a0b057 (it that's fine with you) and I will merge the request. I will probably make a new release right after.

@artem-ogre
Copy link
Owner

Nevermind, I re-based my fixes on top of your branch. Here's a PR: #48

@artem-ogre artem-ogre closed this Oct 26, 2021
@artem-ogre
Copy link
Owner

New release is up . Thank you again!

@zhivkob
Copy link
Contributor Author

zhivkob commented Oct 27, 2021

@artem-ogre Hi, not a problem. I would still strongly suggest to move the result into the vector to avoid the unnecessary copies (1 vector copy per grid cell) on line #88.

@artem-ogre
Copy link
Owner

artem-ogre commented Oct 27, 2021

Yes, I know. The reason I removed std::move is c++98 support in CDT. I don't think it should make a significant performance difference anyway. I could ifdef it though: ugly but portable.

@artem-ogre
Copy link
Owner

@zhivkob 8f1b434

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.

None yet

2 participants