search

wdas / ptex

Per-Face Texture Mapping for Production Rendering https://wdas.github.io/ptex
https://www.disneyanimation.com/open-source/ptex/
Other
681 stars 140 forks source link

In this optimized code, I kept the same structure and functionality, just organized the code cleaner and more readable. #74

Closed BrunoDiego67 closed 1 year ago

betajippity commented 1 year ago

This commit ostensibly "reorganizes" code to be more readable, but upon examining the diff, this commit doesn't even appear to meaningfully reorganize anything. Many (all?) of the changes just replace existing text with exactly the same text, formatted exactly the same way.

Can you explain how this commit isn't just churn for the pure sake of churn? Can you explain why this commit should be merged?

Also, the merge request title mentioned optimizations- given that this commit just replaces each line with exactly the same line of text, all in either README or build files and none in actual code, can you explain what exactly is being optimized here?

davvid commented 1 year ago

It looks like the bulk of the diff is a unix2dos change of LF to CRLF.

Please stick to using unix line endings. You can use dos2unix to convert your files back. You might also want to check your git settings to make sure that you keep the original line endings.

https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings?platform=windows

You might actually be better off using git config --global core.autocrlf input if you're using a modern editor that is able to correctly edit files with unix-style \n LF line endings.

BrunoDiego67 commented 1 year ago

This commit ostensibly "reorganizes" code to be more readable, but upon examining the diff, this commit doesn't even appear to meaningfully reorganize anything. Many (all?) of the changes just replace existing text with exactly the same text, formatted exactly the same way.

Can you explain how this commit isn't just churn for the pure sake of churn? Can you explain why this commit should be merged?

Also, the merge request title mentioned optimizations- given that this commit just replaces each line with exactly the same line of text, all in either README or build files and none in actual code, can you explain what exactly is being optimized here?

This commit ostensibly "reorganizes" code to be more readable, but upon examining the diff, this commit doesn't even appear to meaningfully reorganize anything. Many (all?) of the changes just replace existing text with exactly the same text, formatted exactly the same way.

Can you explain how this commit isn't just churn for the pure sake of churn? Can you explain why this commit should be merged?

Also, the merge request title mentioned optimizations- given that this commit just replaces each line with exactly the same line of text, all in either README or build files and none in actual code, can you explain what exactly is being optimized here?

Well, as I said, I didn’t change anything, I just gave an organized quote

BrunoDiego67 commented 1 year ago

It looks like the bulk of the diff is a unix2dos change of LF to CRLF.

Please stick to using unix line endings. You can use dos2unix to convert your files back. You might also want to check your git settings to make sure that you keep the original line endings.

https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings?platform=windows

You might actually be better off using git config --global core.autocrlf input if you're using a modern editor that is able to correctly edit files with unix-style \n LF line endings.

Thanks for the advice as I am new to programming, have a lot to learn and appreciate your help