Conversation
|
Я буду переписывать тут всё, потому что мне не понравился GLM и я хочу на Eigen переписать. В GLM есть всякие штуки, после которых не ощущается, что я from scratch делаю |
DimaTrushin
left a comment
DimaTrushin
left a comment
There was a problem hiding this comment.
Обрати внимание, что я не повторял замечания. Если одна и та же ошибка всречалась несколько раз, я комментировал лишь единожды. Потому я ожидаю, что ты поправишь все аналогичные замечания всюду.
| project(3d-renderer) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 20) | ||
|
|
There was a problem hiding this comment.
Добавь строчку
set(CMAKE_CXX_STANDARD_REQUIRED True)А то cmake разрешено откатиться на более низкий стандарт.
|
|
||
| namespace renderer { | ||
|
|
||
| Vec3 TransformVector(const Mat4& transformation_matrix, const Vec3& vector) { |
There was a problem hiding this comment.
Если объявляешь методы внутри cpp файлов (а так же классы и переменные), которых нет в h файле, то обязательно заворачивай их в anonymous namespace. Иначе это черевато ошибками линкера, а с ним лучше не играться.
| #include "glm/ext.hpp" | ||
|
|
||
| // I plan to rewrite all of this using Eigen | ||
| // Because glm::perspective and glm::translate feels like cheating to me |
There was a problem hiding this comment.
Можешь их просто не использовать.
|
|
||
| class World { | ||
| public: | ||
| World(const std::vector<Polygon>& polygons) : polygons_(polygons) { |
There was a problem hiding this comment.
- Почему имплементация в хедере
- Плохая идея принимать конкретные контейнеры в конструкторе. Список инициализации тоже плохая идея, так как из него нельзя мувнуть данные, а у тебя полигоны не легкие объекты. Потому либо два итератора, либо по rvalue ссылке.
| World(const std::vector<Polygon>& polygons) : polygons_(polygons) { | ||
| } | ||
|
|
||
| void AddPolygon(const Polygon& polygon); |
There was a problem hiding this comment.
Как минимум нужен метод позволяющий сделать move полигона внутрь.
void AddPolygon(Polygon&& polygon);| } | ||
|
|
||
| Vec3 GetFocalPoint() const; | ||
|
|
There was a problem hiding this comment.
Не делай так много вертикальных пробелов между функциями в h файле.
| : focal_point_(kDefaultFocalPoint), | ||
| near_dist_(kDefaultNearDist), | ||
| far_dist_(kDefaultFarDist), | ||
| fov_y_(kDefaultFOV) { |
There was a problem hiding this comment.
Это делается не так
class Camera {
public:
Camera() = default;
...
private:
focal_point_ = kDefaultFocalPoint;
near_dist_ = kDefaultNearDist;
far_dist_ = kDefaultFarDist;
fov_y_ = kDefaultFOV;
};| static constexpr CoordType kDefaultNearDist = 0.1; | ||
| static constexpr CoordType kDefaultFarDist = 10.0; | ||
| static constexpr Vec3 kDefaultFocalPoint = {0, 0, 0}; | ||
| static constexpr CoordType kDefaultFOV = 45.0; |
There was a problem hiding this comment.
Константы можно перенести в конец перед переменными класса.
| far_dist_(kDefaultFarDist), | ||
| fov_y_(kDefaultFOV) { | ||
| } | ||
| Camera(Vec3 focal_point, CoordType near_dist, CoordType far_dist, CoordType fov_y) |
There was a problem hiding this comment.
Не очень хорошо, что у тебя 2, 3 и 4 аргументы одного типа. Их можно легко перепутать в конструкторе. Тут бы использовать strongly typed aliases, но уже enum-ами ты не обойдешься в случае float или double.
| : focal_point_(focal_point), near_dist_(near_dist), far_dist_(far_dist), fov_y_(fov_y) { | ||
| } | ||
|
|
||
| Vec3 GetFocalPoint() const; |
There was a problem hiding this comment.
Уверен, что хочешь вектор возвращать by value?
DimaTrushin
left a comment
DimaTrushin
left a comment
There was a problem hiding this comment.
Все очень сырое и недоделанное
Тут придется много копать
| void Application::Run() { | ||
| while (window_.isOpen()) { | ||
| timer_.Tick(); | ||
| window_.clear(); |
There was a problem hiding this comment.
Нарушение уровня абстракции. Это низкоуровневая фигня, которая является частью отрисовки фрейма, непонятно зачем это тут зовется. Это должно быть внутри функции draw frame.
| window_.clear(); | ||
| HandleEvents(); | ||
| HandleKeyboard(); | ||
| RenderFrame(); |
There was a problem hiding this comment.
- Слишком специфичное название. У тебя выше две функции общего назначения, которые обслуживают run-time loop. А тут почему-то ты делаешь такую мелкую специфическую работу, как отрендерить фрейм. Это скорее всего должно быть частью какой-то другой общей работы.
- Название метода врет, ты не просто рендеришь фрейм, ты его сначала создаешь, а потом отрисовываешь на экране. Не ври в названии методов.
| HandleEvents(); | ||
| HandleKeyboard(); | ||
| RenderFrame(); |
There was a problem hiding this comment.
Ну это просто феерия черных ящиков. Прям три закрытые шкатулки, угадайте где лежит миллион рублей. Блять, ты что в Поле Чудес переиграл? Как вообще понять, какой тут поток данных? Ты просто манипулируешь скрыто состоянием объекта Application и все. Не делай так.
| void Application::HandleEvents() { | ||
| while (const std::optional event = window_.pollEvent()) { | ||
| if (event->is<sf::Event::Closed>()) { | ||
| window_.close(); |
There was a problem hiding this comment.
Странно, что после этого ты не останавливаешь run-time loop, а делаешь вызовы к двух других функций.
| Vec2 point_to_check = {static_cast<CoordType>(x) + 0.5, | ||
| static_cast<CoordType>(y) + 0.5}; |
There was a problem hiding this comment.
это должно быть преобразование координат
2 participants
No description provided.