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

Devyatovskaya task3 #35

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Devyatovskaya task3 #35

wants to merge 16 commits into from

Conversation

Lexxsage
Copy link
Collaborator

No description provided.

@Lexxsage Lexxsage requested a review from sadads1337 March 22, 2021 07:35
@Lexxsage Lexxsage self-assigned this Mar 22, 2021
@Lexxsage Lexxsage changed the title task3 Devyatovskaya task3 Mar 22, 2021
@Lexxsage
Copy link
Collaborator Author

Lexxsage commented Apr 5, 2021

Оно работает:)


include_directories( ${OPENGL_INCLUDE_DIRS})

qt5_wrap_ui(UI_HEADERS PhongLighting.ui RenderDialog.ui MorphingDialog.ui)
Copy link
Owner

Choose a reason for hiding this comment

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

Нужно поправить cmake аналогично тому, как это сделано в Task2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

поправила

@sadads1337 sadads1337 force-pushed the feature-devyatovskaya-task-3 branch from d1e0a73 to 1b955bb Compare May 17, 2021 01:58
std::string vertex;
std::string fragment;
std::shared_ptr<GLMeshRendererGenerator> renderer_generator;
std::shared_ptr<QOpenGLShaderProgram> shader_program{nullptr};
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
std::shared_ptr<QOpenGLShaderProgram> shader_program{nullptr};
std::shared_ptr<QOpenGLShaderProgram> shader_program;

float shininess;
};

uniform Material material;
Copy link
Owner

Choose a reason for hiding this comment

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

Шейдеры лучше не копировать (чревато ошибками), можно просто вынести общий код в отдельный шейдер и перед компиляцией склеить все шейдеры вместе (как C++ строки)

Можно не исправлять.


class ShaderCollection final {
public:
inline static std::map<std::string, ShaderData> shaders = {
Copy link
Owner

Choose a reason for hiding this comment

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

Это не java :) Зачем вам класс с одной статической переменной :)

void init();

private:
GLScene default_morphing(ShaderData &data);
Copy link
Owner

Choose a reason for hiding this comment

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

Если эти методы не модифицируют ShaderData, то лучше сделать метод константным и данные передавать по const ссылке

Suggested change
GLScene default_morphing(ShaderData &data);
[[nodiscard]] GLScene default_morphing(const ShaderData &data);

Q_OBJECT

public:
PhongLighting(QWidget *parent = Q_NULLPTR);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
PhongLighting(QWidget *parent = Q_NULLPTR);
PhongLighting(QWidget *parent = nullptr);


class MeshGeneratorCollection final {
public:
inline static std::map<std::string, std::shared_ptr<GLMeshGenerator>>
Copy link
Owner

Choose a reason for hiding this comment

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

Опять класс с одной статической переменной

#include <string>

struct GLTexture {
std::string type;
Copy link
Owner

Choose a reason for hiding this comment

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

Лучше использовать enum class


struct GLTexture {
std::string type;
std::string path;
Copy link
Owner

Choose a reason for hiding this comment

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

std::filesystem::path

Copy link
Owner

Choose a reason for hiding this comment

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

И поле вообще не используется

shader_program_ = std::move(shader_program);
}

void GLMeshRenderer::render_wireframe(QOpenGLFunctions_3_0 &functions,
Copy link
Owner

Choose a reason for hiding this comment

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

Просите интерфейс для 3.0 версии, а в шейдерах 450 ставите. Все еще проблема с несколькими точками входа.

float shininess;
bool is_light_source;

GLMaterial(QColor _ambient = {}, QColor _diffuse = {}, QColor _specular = {},
Copy link
Owner

Choose a reason for hiding this comment

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

Нуууу, это какая-то "жесть", задавайте значения полям. А объект создавайте через фигурные скобки.

@sadads1337 sadads1337 force-pushed the feature-devyatovskaya-task-3 branch from 1b955bb to 7f50dae Compare May 24, 2021 03:55
Copy link
Owner

@sadads1337 sadads1337 left a comment

Choose a reason for hiding this comment

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

Почитайте замечания, исправить вы их очевидно не успеете.


struct GLTexture {
std::string type;
std::string path;
Copy link
Owner

Choose a reason for hiding this comment

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

И поле вообще не используется


Q_OBJECT
signals:
void emit_fps(const QString &);
Copy link
Owner

Choose a reason for hiding this comment

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

Сигнал не реализован

public:
void calculate_fps();

float delta_time() const;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
float delta_time() const;
[[nodiscard]] float delta_time() const;

float delta_time() const;

private:
QString fps_to_str() const;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
QString fps_to_str() const;
[[nodiscard]] QString fps_to_str() const;

elapsed_frame_time_ += frame_time;

// update fps every half second
if (elapsed_frame_time_ >= 0.5f) {
Copy link
Owner

Choose a reason for hiding this comment

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

0.5f лучше вынести в отдельную compile-time константу с понятным названием.

void set_morph_factor(int morph_factor);

signals:
void send_fps(const QString &);
Copy link
Owner

Choose a reason for hiding this comment

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

нереализован сигнал

PreparedScenes prep_scenes_;

std::size_t scene_count_{0};
bool is_morphing_{false};
Copy link
Owner

Choose a reason for hiding this comment

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

Никогда не читается

void update_framerate();

public:
PreparedScenes prep_scenes_;
Copy link
Owner

Choose a reason for hiding this comment

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

Снова публичные поля

GLScene default_scene_sphere(ShaderData &);
GLScene default_scene_cube(ShaderData &);

GLScene mesh_earth(ShaderData &);
Copy link
Owner

Choose a reason for hiding this comment

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

Методы ниже не реализованы


QSurfaceFormat format;
format.setSamples(16);
format.setVersion(2, 1);
Copy link
Owner

Choose a reason for hiding this comment

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

Тут 2.1 версию OpenGL просите, в коде создаете контекст и интерфейс для 3.0, в шейдерах просите 4.1+. Странное, что оно вообще не падает

@sadads1337 sadads1337 force-pushed the feature-devyatovskaya-task-3 branch from 7f50dae to a1549d0 Compare May 24, 2021 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants