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

buggy paths #82

Merged
merged 10 commits into from
Jan 10, 2020
Merged

buggy paths #82

merged 10 commits into from
Jan 10, 2020

Conversation

fundies
Copy link
Contributor

@fundies fundies commented Jan 5, 2020

This PR adds support for paths, refactors asset drawing to allow drawing out of bounds, expands the grid in room editor to the whole window, changes room to only redraw on model updates, and probably 3 other things I forgot.

@JoshDreamland JoshDreamland mentioned this pull request Jan 5, 2020
@fundies
Copy link
Contributor Author

fundies commented Jan 7, 2020

@JoshDreamland this ready for review now

@fundies
Copy link
Contributor Author

fundies commented Jan 7, 2020

This does crash seemingly randomly when I spam the delete button sometimes with qvector out of range error but I cant reproduce it regularly


#include <QScrollArea>

class AssetScrollArea : public QScrollArea {
Copy link
Member

Choose a reason for hiding this comment

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

In the not-too-distant future: please add middle-click panning to this widget.

Copy link
Contributor Author

@fundies fundies Jan 8, 2020

Choose a reason for hiding this comment

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

You can pan with the keyboard. If you want mouse panning feel free to amend this pr or you can create an issue so someone else can add it later. It's not a priority for me.

Copy link
Member

Choose a reason for hiding this comment

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

I worry that's not discoverable. These editors are primarily mouse-driven.

Widgets/AssetScrollAreaBackground.cpp Outdated Show resolved Hide resolved
Widgets/AssetScrollAreaBackground.cpp Outdated Show resolved Hide resolved
field(m->protobuf->GetReflection()
->GetMutableRepeatedFieldRef<Message>(m->protobuf, m->field)) {}
RowRemovalOperation(RepeatedProtoModelPtr m)
: model(m), field(m->protobuf->GetReflection()->GetMutableRepeatedFieldRef<Message>(m->protobuf, m->field)) {}
Copy link
Member

Choose a reason for hiding this comment

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

Your clang-format config seems to have a long line width set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So?

Copy link
Member

Choose a reason for hiding this comment

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

So it's taking code that was wrapped to 80-char limits (the norm) and re-wrapping it to something more like 200.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have 120 set. Robert and I felt 80 was too low

@@ -159,6 +160,14 @@ bool RepeatedProtoModel::insertRows(int row, int count, const QModelIndex &paren
return true;
}

void RepeatedProtoModel::RecursiveDataChanged() {
Copy link
Member

Choose a reason for hiding this comment

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

Technically this function's not recursive... it's traversing an inheritance hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok transvestite datachanged or whatever it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whats the actual word for this?

Copy link
Member

Choose a reason for hiding this comment

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

Iterative, but you probably just want to call it DataChangedThroughHierarchy or DataChangedForParents or something.

Widgets/AssetScrollAreaBackground.h Outdated Show resolved Hide resolved
gridWidth, gridHeight);
if (model->data(Background::kUseAsTilesetFieldNumber).toBool()) {
grid.show = true;
grid.horSpacing = model->data(Background::kHorizontalSpacingFieldNumber).toInt();
Copy link
Member

Choose a reason for hiding this comment

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

In the future, it might be cool to allow binding the ScrollAreaBackground to these fields, directly. Especially if we add a UI-specific field to paths for persisting grid settings, as we've done for rooms.

Incidentally, maybe we should consider extracting those into a file we can .gitignore, eg, roomname.rm.ide.

Widgets/RoomView.cpp Show resolved Hide resolved
Widgets/RoomView.cpp Show resolved Hide resolved

class ResourceSelector : public QToolButton {
public:
explicit ResourceSelector(QWidget* parent, TreeNode::TypeCase type = TreeNode::TypeCase::kObject)
Copy link
Member

Choose a reason for hiding this comment

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

I feel dirty defaulting this to object. This is just so this constructor can serve as the default, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without a default argument the designer's generated code craps out. I can default the type to none if you prefer but I do need a default

Copy link
Member

@JoshDreamland JoshDreamland left a comment

Choose a reason for hiding this comment

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

The way you implemented room rendering in the path editor is pretty cool. I will think about how to use this when we go to add Overworlds. I think we'll need to add a caching layer to it, so that when rendering at less than 25% scale, only the cached version is used.

Anyway, if you're done, I'll merge this.

@@ -80,7 +82,8 @@ void BackgroundEditor::on_actionLoadImage_triggered() {
// QString newData = GetModelData(Background::kImageFieldNumber).toString();
// TODO: Copy data into our egm and reset the path
// SetModelData(Background::kImageFieldNumber, lastData);
} else qDebug() << "Failed to load gmx Background";
} else
qDebug() << "Failed to load gmx Background";
Copy link
Member

Choose a reason for hiding this comment

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

How strange... I thought we were using Google's. Have they pushed out an update?

}

PathEditor::~PathEditor() { delete ui; }

void PathEditor::RebindSubModels() {
pathModel = _model->GetSubModel(TreeNode::kPathFieldNumber);
Copy link
Member

Choose a reason for hiding this comment

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

I see. So these are (or at least _model is) a weak pointer, and the editor will simply close when this would otherwise go stale. Sounds fine, although might lead to annoying behavior around, eg, undoing and redoing add or delete operations.

}
}
}
return QWidget::eventFilter(obj, event);
Copy link
Member

Choose a reason for hiding this comment

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

I should clarify: I get why we call it in general, but I'm not sure we should be calling it when we've already handled the event. But assuming you're seeing the right behavior for events we are handling here, this should be fine.

Widgets/RoomView.cpp Show resolved Hide resolved
@fundies
Copy link
Contributor Author

fundies commented Jan 8, 2020

The way you implemented room rendering in the path editor is pretty cool. I will think about how to use this when we go to add Overworlds. I think we'll need to add a caching layer to it, so that when rendering at less than 25% scale, only the cached version is used.

Anyway, if you're done, I'll merge this.

I would like to rename your "shitty curve" functions can you come up with better names and I will rename that plus my datachanged crap. then should be good to merge

@JoshDreamland
Copy link
Member

We can use "approximate curve" or something.

@JoshDreamland
Copy link
Member

So apparently control-enter closes and comments...?

@JoshDreamland JoshDreamland merged commit 4b0c8aa into enigma-dev:master Jan 10, 2020
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.

2 participants