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

Changelog: #3

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

Chuvi-w
Copy link

@Chuvi-w Chuvi-w commented Aug 18, 2022

  1. Add CMake support. (.sln and .vsxproj could be deleted)
  2. Move DumpSTL.h to include/DumpSTL.h directory to prevent adding other library dirs and files to include scope.
  3. Replace std::string and std::string_view to std::filesystem::path, when we're talking about path.
  4. Some more fixes...

1. Add CMake support. (.sln and .vsxproj could be deleted)
2. Move DumpSTL.h to include/DumpSTL.h directory to prevent adding other library dirs  and files to include scope.
3. Replace std::string and std::string_view to std::filesystem::path, when we're talking about path.
4.  Some more fixes...
@Chuvi-w Chuvi-w closed this Aug 18, 2022
@Chuvi-w Chuvi-w reopened this Aug 18, 2022
Copy link
Collaborator

@sergey-vaytsel sergey-vaytsel left a comment

Choose a reason for hiding this comment

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

пробегал тут мимо, поревьюил 👍

CMakeLists.txt Outdated Show resolved Hide resolved
include/DumpSTL/DumpSTL.h Outdated Show resolved Hide resolved
include/DumpSTL/DumpSTL.h Outdated Show resolved Hide resolved
include/DumpSTL/DumpSTL.h Outdated Show resolved Hide resolved
include/DumpSTL/DumpSTL.h Outdated Show resolved Hide resolved
include/DumpSTL/DumpSTL.h Outdated Show resolved Hide resolved
}
void exportBin(std::string&& str) const {
void exportBin(fs::path&& filename) const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

эта реализация теперь дублирует вариант выше

include/DumpSTL/DumpSTL.h Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Chuvi-w and others added 2 commits August 22, 2022 08:55
Co-authored-by: Sergey Vaytsel <sergey.vaytsel@gmail.com>
Co-authored-by: Sergey Vaytsel <sergey.vaytsel@gmail.com>
Co-authored-by: Sergey Vaytsel <sergey.vaytsel@gmail.com>
@Chuvi-w
Copy link
Author

Chuvi-w commented Aug 22, 2022

Дико извиняюсь, принимать предложения по изменениям с телефона на ходу было не лучшей идеей. буду за компьютером - продолжу.

Chuvi-w and others added 4 commits August 22, 2022 13:20
Co-authored-by: Sergey Vaytsel <sergey.vaytsel@gmail.com>
Co-authored-by: Sergey Vaytsel <sergey.vaytsel@gmail.com>
Co-authored-by: Sergey Vaytsel <sergey.vaytsel@gmail.com>
@@ -0,0 +1 @@
add_subdirectory(example)
Copy link
Collaborator

Choose a reason for hiding this comment

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

no newline at end of file тут и в example/example/CMakeLists.txt

Copy link
Author

Choose a reason for hiding this comment

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

Не вижу в синтаксисе cmake требований ставить newline в конце файла.

Copy link
Collaborator

Choose a reason for hiding this comment

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

вот статейку нашёл на эту тему — https://semakin.dev/2020/05/no_newline_at_end_of_file/

некоторые редакторы можно настроить, чтобы автоматом добавлять перенос в конце файла перед сохранением.

enum
{
header_size_bytes = 80
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

думаю проще static constexpr unsigned header_size_bytes = 80;
со структурой приятнее выглядит, согласен

fullFileName << DUMP::folderSTL << fileName << fileNameSeparator << index
<< fileExtension;
fs::path fullFileName;
auto generateFName = [&index, &fileName, &fullFileName]() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

пробела не хватает :)

Suggested change
auto generateFName = [&index, &fileName, &fullFileName]() {
auto generateFName = [&index, &fileName, &fullFileName]() {

header_size_bytes is static constexpr now.
Space added.
constexpr bool isEnable = true; // global turn off/turn on this tool (without clear code)
constexpr std::string_view folderSTL = "C:\\Repos\\STL\\"; // folder, where files will save
Copy link
Owner

Choose a reason for hiding this comment

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

Писал ранее коммент, хз куда пропал, продублирую
Для пользователя каждый раз прописывать полный путь - куда ложить файлы - крайне не удобно

@KupchishinAB KupchishinAB self-requested a review December 3, 2022 19:24
@sergey-vaytsel
Copy link
Collaborator

sergey-vaytsel commented Dec 3, 2022

Мы тут поговорили за чаем, и я осознал (мне объяснили) что тут не нужен CMake и всё присущее нормальной open-source библиотеке. DumpSTL изначально задуман как штука, которую используют для быстрой отладки в любом проекте, на vsproj, cmake, на чём угодно работающих. Пользователь не должен добавлять её как зависимость, устанавливать через conan или ещё как. Это именно грубое такое решение для любых проектов, когда надо на коленке проверить что весь твой код обработки модели работает правильно.
Закинул файл в проект, добавил инклуд, дописал конверторов из структур данных проекта (прям в DumpSTL.h, да-да-да), добавил дебажных вызовов, запустил. Потом запустил скрипт из /scripts, получил красивые картинки моделей в разных проекциях, посмотрел глазами как "быстрые тесты на коленке" прошли. Удалил код, выложил на ревью. Done.
Актуально, когда у тебя меняются проекты, везде разные системы сборки, везде разные способы добавления зависимостей, а тебе нужно просто посмотреть как код отработал сейчас прям.

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

3 participants