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

[unreviewed] [git broken?] Added brush buttons for terrain texture editing #1665

Closed
wants to merge 30 commits into from

Conversation

Projects
None yet
5 participants
@unelsson
Copy link
Contributor

commented Apr 2, 2018

Brush buttons for texture editing mode. Simple icons as placeholders. Only buttons at this time, no functionality at all yet. Please check before merging, my c++ skills and knowledge of this code is limited.

Code based on: #1414 , editmode and instancemode-code.
Feature request on bug tracker: https://bugs.openmw.org/issues/3870
Forum: https://forum.openmw.org/viewtopic.php?f=7&t=4322&start=40#p53377

kcat and others added some commits Sep 20, 2017

@akortunov

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2018

For what purpose did you add a lot of empty lines into pagedworldspacewidget.cpp? Without them, this PR would be a much smaller and cleaner.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2018

Oh.. I thought I cleaned that. I used different editing app once when cleaning my git repo and working with the code, and that app added lines...

@zinnschlag

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

Please fix the issue with the pagedworldspacewidget file. Having random cosmetic changes in a commit makes reviewing on github harder and also introduce possible conflicts. I will have a closer look after you have cleaned up.

I guess TERRAINTEXTURESUBMODE is just a placeholder text?

btw. its a good idea to open a PR early. This way we can give feedback and also keep track of things. We are probably not going to merge until the feature is complete (or at least a large subsection of it). Keeping a PR open for a longer period of time is not a problem.

unelsson and others added some commits Apr 2, 2018

@zinnschlag

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

That is better.

Hm, not quite what I had in mind, but the approach is interesting. Better usability and consistency than my original concept. There is one big problem though. Two of the brush modes require the user to set a size. We currently don't have any GUI element that could handle that and right now I can't come up with anything that wouldn't by clumsy or widely inconsistent with the rest of the GUI. Any ideas?

@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

I'm thinking that the tool shape is selected through that submode, then there should either be an extra toolbar and submodes for textures or maybe better - a window which has all textures and size-slider or box. Something like a QSlider and a box to set the value similar as here: http://doc.qt.io/qt-5/qtwidgets-widgets-sliders-example.html ? Is it necessary to use specific opencs-gui elements instead of just default qt?

@zinnschlag

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

I'm thinking that the tool shape is selected through that submode, then there should either be an extra toolbar and submodes for textures

Problematic. We are very tight on screen-space, since the 3D subview will typically be used along with several other subviews at the same time. Another toolbar is out of question. And since we already have a selection button and a brush button adding another one for the textures is problematic too.

a window which has all textures and size-slider or box

That was how I envisioned the purpose of the brush button originally (displaying the texture/shape and opening this window). Well, minus the textures. We have a subview for textures. No point in duplicating it.

Is it necessary to use specific opencs-gui elements instead of just default qt?

No. But it needs to be consistent and it needs to be compact.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

Does existing texture view have a preview for texture, should there be? Which one is it btw? I found two tables for textures, but not sure which one are you referring to.

To increase usability, possibility is to have a texture overlay to brush shape button. I already tried the overlay, but I haven't yet found the texture data or how it's tied to selected cells.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

Does existing texture view have a preview for texture, should there be?

Nope. Not overly important anyway. Probably a post 1.0 feature. Or we may add it as part of the palette feature, which is a separate task.

Which one is it btw? I found two tables for textures, but not sure which one are you referring to.

The table CSMWorld::UniversalId::Type_LandTextures contains a list of textures currently in use for terrain, i.e. they are index to be referenced by the land texture grid.

There is also CSMWorld::UniversalId::Type_Textures, which lists all bitmap resources via their filename.

We need to handle both for things like dragging a texture to the button. The former simply selects a list. The later would have to create a new LandTexture record in addition.

To increase usability, possibility is to have a texture overlay to brush shape button.

Makes sense.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2018

I've been researching how textures could be dragged to a brush-button, so that the button could hold the data for texture selection. This current implementation (brush buttons in toolbar, scenetoolmode) requires some extra work to make the buttons accept drops.

Regarding this first implementation - I have trouble making class ModeButton to accept dragging anywhere, and I don't know why (button size perhaps?). Even if that could be done, SceneToolMode has framework for ModeButton, and there has to be a way to define whether button accepts drops or not - doing that would require altering ModeButton, perhaps adding a custom variable like mDropAccepted. One has to be careful in altering class ModeButton, because it's a generic class for many buttons with different functions. Creating a BrushButton class for use in toolbar (SceneToolMode) is possible, but requires altering SceneToolMode and possible SceneToolBar, as they are currently coded for using ModeButton only.

I've tested another way to implement this - the one that Zinnschlag originally planned - to open up a window with separate buttons. Then it's possible to create a class BrushButton that doesn't interfere with SceneToolMode or ModeButton. User interface will be quite different however.

So, which road to choose?

@zinnschlag

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

The main problem with the former option is that there is still no place in the GUI to set the brush size. We haven't found a solution to that yet, right? If that continues to be the case then this option won't work. Otherwise I would consider it preferable.

unelsson added some commits Apr 6, 2018

Removed unnecessary include
Accident in previous commit...
@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

An option for implementing brushes in the toolbar - adding a config button that opens a window/frame for advanced settings (e.g. brush size). Pros: Some settings in toolbar, more space for worldview, visually consistent. Cons: Both views might be needed most of the time, so even less space.

It might be useful to have 3-4 (or more?) customizable presets in toolbar, each holding the brush size and shape, perhaps texture? Probably a post 1.0 feature though.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2018

An option for implementing brushes in the toolbar - adding a config button that opens a window/frame for advanced settings (e.g. brush size). Pros: Some settings in toolbar, more space for worldview, visually consistent. Cons: Both views might be needed most of the time, so even less space.

That we can not do. We have the brush button on the toolbar and the entire feature needs to be anchored on this button. Adding more (and different kinds of) buttons to the toolbar would take too much space and would also result in an inconsistent cluttered looking interface.

unelsson and others added some commits Apr 8, 2018

scrawl
scrawl
scrawl
scrawl
scrawl
scrawl
@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

I've been developing code for texture overlay to brush buttons, but it relies on software decompression of texture format that is only supported by OpenMW's OSG branch or OSG version >= 3.6. https://forum.openmw.org/viewtopic.php?f=6&p=53745#p53745

I'm doubtful of committing that feature here, as OpenMW's own OSG isn't the main branch, and therefore this could result in non-wanted behavior on many systems.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

The overlay feature now works on my local branch, but it's running on OpenMW's osg branch. I haven't committed these changes yet, should I? Should there be a check for OSG version, how can I check if system is running on OpenMW's osg instead of mainstream?
https://youtu.be/R_OOvdDQSWM

edit: Putting this feature on hold. If someone is interested in code, it's here https://github.com/unelsson/openmw/tree/terraintexturebrushoverlay (not properly cleaned up code, but it works)

scrawl
@zinnschlag

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

I see you already put it into a separate branch. Good. Let's stick with that. We can always merge once we have moved forward with OSG.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2018

Huh, I merged OpenMW master to my own branch just to stay updated with the newest, but now this says I have changed a lot of files that I really didn't even touch.

Should I make a new pull request or what is going on?

Apparently during this time, instructions for git were changed - eg. prefer rebase over merge. I didn't know that... Would this problem be solved by rebasing?

@unelsson unelsson changed the title Added brush buttons for terrain texture editing [unreviewed] [git broken?] Added brush buttons for terrain texture editing Apr 15, 2018

@zinnschlag

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2018

Hm, not quite sure what is going on here. But could could try to undo the merge and then do a rebase anyway.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

Was easier to just make another PR clean and rebased
#1679

@unelsson unelsson closed this Apr 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.