Add Items and Inventory system #28

gosha-krovsh · 2021-04-15T17:35:41Z

Fixes #25, Fixes #7

Добавлены инвентари специальным объектам и герою.
Добавлены ItemBar и возможность передачи вещей от героя к объектам и обратно.

Персонажи и специальные объекты теперь могут хранить в себе Item-ы Добавлен ItemBar для отображения предметов и героя и возможность передачи вещами между объектами

gosha-krovsh

Нашел баг анимации передачи элементов, в процессе фикса.

ArseniyTy

Я, честно говоря, сильно устал это ревьюить. Под конец вообще запутался из-за сложных связей. 500 строк это слишком много. Надо разбивать такие задачи на более мелкие PR.

Resources/resources.qrc

Styles/styles.qss

View/view.cpp

GameObject/hero.h

View/item_bar.cpp

Controller/controller.cpp

ArseniyTy · 2021-04-16T16:10:16Z

View/view.cpp

+    case 0: return std::make_pair(hero_item_bar_, object_item_bar_);
+    case 1: return std::make_pair(object_item_bar_, hero_item_bar_);
+    default:
+      return std::make_pair(nullptr, nullptr);


Я думал id бывают не только двух типов. Но если так, то почему это не enum?

Цилическое включение вырисовывается, пока оставил int.
Нужно прописать тип 'enum' в 'AbstractController' а в него нельзя include-ать ничего из 'View'.

Этот enum же по сути можно прописать в item_bar

Так я так и прописывал. ItemBar incudes AbstractController, а AbstractController теперь нужно тоже знать этот enum, но нельзя include-уть 'ItemBar' в AbstractController, потому что будет циклическое включение.

Тогда создай enum отдельным файлом и подключай и туда, и туда. Нынешняя система максимально запутывает

Добавлен класс BarPack, который позволяет сделать правильное поведение при изменении размеров окна.

# CMakeLists.txt # Controller/controller.cpp # Controller/controller.h # GameObject/hero.h # Model/constants.h # Model/model.cpp # Model/model.h # Resources/resources.qrc # View/view.cpp # View/view.h

HydeHunter2

пока что ревью на саму механику, а не по коду:

есть бага, что не закрывается менюшка с предметами из сундука, когда ты от него отходишь

ArseniyTy

Прошу также ответить на моё прошлое ревью. Некоторые важные комментарии там остались без внимания!

View/bar_pack.cpp

Controller/abstract_controller.h

View/view.cpp

View/item_bar.cpp

GameObject/object.cpp

GameObject/storage.h

Controller/controller.h

ArseniyTy · 2021-04-28T13:26:45Z

GameObject/object.h


 class Object {
 public:
  enum class Type {
    kNone,
    kFloor,
-    kWall
+    kWall,
+    kStorable


Странно получается, что свойство "хранение вещей" обрабатывается как определённый тип объекта. Т.е. у нас получается, что сундук и герой это объекты одного типа. Мб лучше создать метод IsStorable, который возвращает тру, если хранилище не nullptr. Конечно тогда не получится заюзать метод для поиска объекта в том виде, что он есть сейчас. Но можно тогда его вообще обобщить: передавать предикат, по которому ищется объект

Ох, не хочется так этот метод пределывать. У нас нет 'типа' объекта, который может хранить вещи, это правда, но это так чтобы не было проблем с dynamic_cast и множественным наследование.

Но так оставлять нельзя, так как если мы захотим, чтобы что-то имело тип и при этом хранило вещи то придется хранить два type(??). Это странно, предлагаю ввести просто еще enum характеристик и передавать в поиск еще и его. Тогда можно искать либо по типо, либо по хар-кам, либо по всему сразу.

Обобщить метод на искание по предикату делается очень легко в любом случае. Что касается характеристик: мне не очень понятно, какие ещё характеристики могут быть? Если их 1-2 штуки, то мб нет смысла вводить дополнительные сложности в архитектуру

Сделал метод с предикатом

View/view.cpp

Controller/abstract_controller.h

Zzzabiyaka · 2021-05-05T13:49:44Z

Controller/controller.cpp

+      auto block = map.GetBlock(x, y, hero.GetRoundedZ());
+
+      if (block && predicate(block)) {
+        double distance_squared = (hero_coords.x - x) * (hero_coords.x - x) +


Это потом надо будет поправить. Уже добавили этот функционал

Главное не забудьте. Оставьте TODO

Оставил todo

Zzzabiyaka · 2021-05-05T13:56:16Z

View/bar_pack.h

+          QWidget* parent = nullptr,
+          const std::shared_ptr<Storage>& hero_storage = nullptr,
+          const std::shared_ptr<Storage>& object_storage = nullptr,
+          int center_x = constants::kWindowWidth / 2,
+          int y = 3 * constants::kWindowHeight / 5,
+          int width = constants::kWindowWidth / 2,
+          int height = 2 * constants::kWindowHeight / 5);


Предлагаю идею с вынесением хотя бы части рандомных констант в constants::

Мне эта идея кажется не всегда оправданной, но я вынес и оно впринципе нормально.

Zzzabiyaka · 2021-05-05T13:56:43Z

View/item_bar.cpp

+                   controller_(controller),
+                   storage_(storage),
+                   layout_(new QHBoxLayout()) {
+  for (int i = 0; i < constants::kMaxElementsInItemBar; i++) {


А зачем?)

Zzzabiyaka · 2021-05-05T13:57:39Z

View/item_bar.cpp

+
+  int size = std::min(static_cast<int>(storage_->GetItems().size()),
+                      constants::kMaxElementsInItemBar);
+  for (int i = 0; i < size; i++) {


Zzzabiyaka · 2021-05-05T13:58:03Z

View/item_bar.cpp

+}
+
+void ItemBar::ClearIconsFromIndex(int index) {
+  for (int i = index; i < constants::kMaxElementsInItemBar; i++) {


Zzzabiyaka · 2021-05-05T13:58:35Z

View/item_bar.cpp

+
+void ItemBar::SetButtonsSize(int width, int max_height) {
+  int size = std::min(width / constants::kMaxElementsInItemBar, max_height);
+  int space_between_buttons = size / 25;


+1 про рандомные константы. Глянь сам какие там удобнее вынести будет

Zzzabiyaka · 2021-05-05T13:58:50Z

View/item_bar.cpp

+  size -= space_between_buttons;
+  for (auto& button : buttons_) {
+    button->setFixedSize(size, size);
+    button->setIconSize(button->size() * 0.8);


+2 random constants

Zzzabiyaka · 2021-05-05T13:59:57Z

View/view.cpp

+    case Qt::Key_7 : {
+      item_bar_pack_->GetHeroBar()->UseItem(event->key() - Qt::Key_1);
+      break;
+    }
  }
 }


Ещё бы кнопочку, чтобы быстро добавить на первую свободную позицию в инвентарь

Я не очень понял о чем ты

Zzzabiyaka · 2021-05-05T14:00:37Z

View/view.cpp

+                                  item_bar_pack_->GetObjectBar());
+    case 1: return std::make_pair(item_bar_pack_->GetObjectBar(),
+                                  item_bar_pack_->GetHeroBar());
+    default:return std::make_pair(nullptr, nullptr);


не ну это жесть. Хоть пробел вставь, а лучше, имхо, {...}

да, тут не по кодстайлу, конечно

Сделано

elizabethfeden

статики плохо, класс сундука плохо, все остальное ок

опять-таки только комментирую, чтобы сами разбирались и процесс шел быстрее

elizabethfeden · 2021-05-05T19:00:34Z

Controller/controller.cpp

@@ -133,6 +140,38 @@ Object* Controller::FindNearestObjectWithType(Object::Type type) {
  return nearest_block;
 }

+Object* Controller::FindIfNearestObject(


нужно тогда вызывать FindWithType() через этот метод, чтобы не было дублирования кода

Сделано

elizabethfeden · 2021-05-05T19:04:11Z

GameObject/chest.h

+  static void DeleteImage();
+
+ private:
+  static std::shared_ptr<QPixmap> chest_image_;


что за статики -___-
мы разве не решили делать без них и во всех других местах должно быть без статиков?

Да да, этот PR просто настолько стар, что та система без static была не слита и я сделал временно так. Забыл пометить что это временный код

elizabethfeden · 2021-05-05T19:07:37Z

GameObject/chest.h

+#include "object.h"
+#include "storage.h"
+
+class Chest : public Object {


хз зачем вообще этот класс, он по сути нужен только чтобы картинку показывать. можно же просто сделать Storage с картинкой, а не плодить лишние классы

Просто объектами, которые могут что-то хранить бывают Hero и (возможно)Bot. Поэтому я сделал Storage чисто инвентарем, а в качесте блока, который может что-то хранить, выступает Chest

elizabethfeden · 2021-05-05T19:07:59Z

GameObject/item.cpp

+#include "item.h"
+
+Item::Item(int id, const QString& name, const QPixmap& image) :
+    name_(std::move(name)),


можно не мувать

elizabethfeden · 2021-05-05T19:10:58Z

View/view.cpp

+                                  item_bar_pack_->GetObjectBar());
+    case 1: return std::make_pair(item_bar_pack_->GetObjectBar(),
+                                  item_bar_pack_->GetHeroBar());
+    default:return std::make_pair(nullptr, nullptr);


да, тут не по кодстайлу, конечно

PGSStas · 2021-05-05T19:21:14Z

Controller/controller.cpp

+
+void Controller::OnItemPress(int bar_id, int index) {
+  std::pair<ItemBar*, ItemBar*> source_dest = view_->GetSrcDestBars(bar_id);
+  if (source_dest.first && source_dest.second) {


Напиши явное сравнение с nullptr, а то с булом можно попутать.

Сделано

PGSStas · 2021-05-05T19:23:21Z

GameObject/hero.cpp

@@ -1,7 +1,9 @@
 #include "hero.h"

 Hero::Hero(const Point& coords)
-  : Creature(coords, "Hero", constants::kHP) {}
+  : Creature(coords, "Hero", constants::kHP) {
+  storage_ = std::make_shared<Storage>();


А чего не списком инициализации?

Потому что Storage есть в всех объектов, просто у каких-то он nullptr, а у каких-то существует. Тут получается он инициализируется при вызове конструктора Creature.

PGSStas · 2021-05-05T19:26:09Z

View/bar_pack.cpp

+    hero_bar_(new ItemBar(0, controller, this, hero_storage)),
+    object_bar_(new ItemBar(1, controller, this, object_storage)) {


Блин, ну вот это точно выглядит, что нужно перейти на умные указатели.

Просто в Qt части мы привыкли использовать сырые указатели, поэтому так и написано. Может поэтому можно так оставить?

PGSStas · 2021-05-05T19:26:46Z

View/item_bar.cpp

+                   controller_(controller),
+                   storage_(storage),
+                   layout_(new QHBoxLayout()) {
+  for (int i = 0; i < constants::kMaxElementsInItemBar; i++) {


А зачем?)

PGSStas · 2021-05-05T19:28:33Z

View/item_bar.cpp

+  int size = std::min(static_cast<int>(storage_->GetItems().size()),
+                      constants::kMaxElementsInItemBar);
+  for (int i = 0; i < size; i++) {
+    QPixmap image = storage_->GetItems().at(i).GetImage();


QString не кинет исключение, если там выход за пределы?

А к чему это?

PGSStas · 2021-05-05T19:29:33Z

View/item_bar.cpp

+}
+
+void ItemBar::UseItem(int) {
+  // todo::make items usable


TODO принято капсом писать, вроде бы. @elizabethfeden не помнишь?

PGSStas · 2021-05-05T19:30:44Z

View/item_bar.cpp

+}
+
+void ItemBar::resizeEvent(QResizeEvent* event) {
+  layout_->setSpacing(static_cast<int>(0.01 * width()));


Кажется, что тут можно сделать width() / 100 без каста, и получить ровно тот же результат. Раз эта константа никуда не вынесена, то это скорее всего постоянное изменение.

Сделано

PGSStas · 2021-05-05T19:32:50Z

CMakeLists.txt

+        View/item_bar.cpp
+        View/bar_pack.cpp


Отсортите и тут по алфавиту)

Сделано

PGSStas · 2021-05-05T19:33:41Z

Controller/controller.cpp

+  Object* nearest_storage = FindIfNearestObject([](Object* block) {
+    return block->IsStorable();
+  });
+  if (view_->IsItemDialogOpen() && !nearest_storage) {


Также сделайте явную проверку на nullptr

Сделано

PGSStas · 2021-05-05T19:34:29Z

Controller/controller.cpp

+      auto block = map.GetBlock(x, y, hero.GetRoundedZ());
+
+      if (block && predicate(block)) {
+        double distance_squared = (hero_coords.x - x) * (hero_coords.x - x) +


Главное не забудьте. Оставьте TODO

# Controller/abstract_controller.h # Controller/controller.cpp # Controller/controller.h # Controller/data_controller.cpp # GameObject/hero.h # Model/constants.h # Resources/resources.qrc # View/view.cpp # View/view.h

gosha-krovsh added 2 commits April 15, 2021 20:10

Fix style

0d38b52

gosha-krovsh added the enhancement New feature or request label Apr 15, 2021

gosha-krovsh requested review from HydeHunter2, PGSStas, elizabethfeden, ArseniyTy and Zzzabiyaka April 15, 2021 17:35

gosha-krovsh self-assigned this Apr 15, 2021

gosha-krovsh commented Apr 16, 2021

View reviewed changes

ArseniyTy requested changes Apr 16, 2021

View reviewed changes

gosha-krovsh and others added 7 commits April 27, 2021 20:27

Fix resize bug and some smaller ones

9cad4a9

Добавлен класс BarPack, который позволяет сделать правильное поведение при изменении размеров окна.

Fix file issues

3cd1f0d

# Conflicts:

49778da

# CMakeLists.txt # Controller/controller.cpp # Controller/controller.h # GameObject/hero.h # Model/constants.h # Model/model.cpp # Model/model.h # Resources/resources.qrc # View/view.cpp # View/view.h

Remove dynamic_cast

c22b4a1

Merge branch 'master' into add-items

da47c51

Fix cpplint

fd9315c

Fix cpplint

0142a24

HydeHunter2 requested changes Apr 27, 2021

View reviewed changes

Fix bug with closing a second ItemBar

b44d96e

gosha-krovsh requested a review from HydeHunter2 April 28, 2021 09:13

Fix arrow-keys overlapping bug and buttons' styling

daa3721

ArseniyTy requested changes Apr 28, 2021

View reviewed changes

gosha-krovsh added 3 commits April 29, 2021 22:09

Fix styling

2073d80

Fix small issue

11b349f

Fix bugs

e35ff31

ArseniyTy reviewed May 2, 2021

View reviewed changes

Controller/abstract_controller.h Outdated Show resolved Hide resolved

Fix const reference

5ad7dec

Zzzabiyaka reviewed May 5, 2021

View reviewed changes

elizabethfeden reviewed May 5, 2021

View reviewed changes

PGSStas requested changes May 5, 2021

View reviewed changes

gosha-krovsh added 2 commits May 6, 2021 21:49

Fix bugs

fea157d

Fix Style

f64288c

PGSStas previously approved these changes May 8, 2021

View reviewed changes

ArseniyTy previously approved these changes May 8, 2021

View reviewed changes

gosha-krovsh added 2 commits May 9, 2021 21:46

# Conflicts:

ef1ced6

# Controller/abstract_controller.h # Controller/controller.cpp # Controller/controller.h # Controller/data_controller.cpp # GameObject/hero.h # Model/constants.h # Resources/resources.qrc # View/view.cpp # View/view.h

Fix merge bugs

0c36663

gosha-krovsh dismissed stale reviews from ArseniyTy and PGSStas via 0c36663 May 9, 2021 19:12

gosha-krovsh and others added 2 commits May 9, 2021 22:19

Merge branch 'master' into add-items

e571855

Fix merge bugs

e0a1a86

ArseniyTy approved these changes May 9, 2021

View reviewed changes

HydeHunter2 approved these changes May 9, 2021

View reviewed changes

gosha-krovsh merged commit a924d24 into master May 9, 2021

gosha-krovsh deleted the add-items branch May 9, 2021 21:01

		hero_bar_(new ItemBar(0, controller, this, hero_storage)),
		object_bar_(new ItemBar(1, controller, this, object_storage)) {

Add Items and Inventory system #28

Add Items and Inventory system #28

Conversation

gosha-krovsh commented Apr 15, 2021

gosha-krovsh left a comment

Choose a reason for hiding this comment

ArseniyTy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HydeHunter2 left a comment

Choose a reason for hiding this comment

ArseniyTy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elizabethfeden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment