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

Editor: Transient land shape editing #2520

Merged
merged 46 commits into from Oct 24, 2019

Conversation

@unelsson
Copy link
Contributor

unelsson commented Sep 11, 2019

New version of #2167 to make reviewing easier. The previous PR had terrain selection changes included in the changelog.

Terrain editing feature limits the edits to the range of ESM file format. This PR now also implements a feature that highlights the broken land heights as red whenever the edit is about to go out of range.

Features:

  • Terrain vertex selection.
  • Terrain shape editing
  • Transient land shape editing (press esc to undo). Changes made to land are held in float array until the drag end. Memory is not allocated runtime, but rather all cells constantly hold an extra float array. This feature overrides a function from Storage, shouldn't affect OpenMW game engine.
  • quicker land height calculations (affects also terrain selection)

TODO:

  • Fix cell edges breaking, when cells are not pre-loaded.
  • Add drag-amount to heights change
  • Proper naming of variables and functions in alterheights and heightmap to alterheight and height
  • Limit land edits to steps of +-8, in order to avoid land tearing because of rounding when saving.
  • Limit slopes to fit land format (extreme changes will break the save)
  • Develop basic tools for land editing
  • Fix bug: saves break, investigate when and why (reason: missing ltex records)
  • Fix bug: limitHeightChanges function breaks land, remove duplicated code
  • Fix bug: limitHeightChanges still rarely breaks land corners and edges
  • Fix bug: Out-of-limits land-edits, strange hit values?
  • Fix bug: Land normals command is issued after land shape command (resulting in wrong normals)
  • Fix bug: Tool size 1 doesn't do anything with circle/square brush
  • Fix bug: Slope steepness limiter, while redesigned, is still failing at some extreme cases. Workaround (don't let failed limit efforts pass).
  • Fixed bug: Square tool breaks cell edges (alterHeight bug)
  • Fix bug (5177): Mini-map gets corrupted
  • Fix bug: Brush button doesn't get deleted.
  • Fix bug: Land limiting fails when new land is automatically created at the edge of the world. New lands are created at height 0, and there is an automatic cell edge break. New lands should be automatically connected to valid edging land.
  • Fix bugs: Some cells (e.g. -1, -17 in Morrowind) has a land record, but no land is visible, yet terrain heights are set to zero - currently dragging a land edit to a cell like this will result in a cascade of failure.
  • Improve: Generate new minimap (mWnam record) based on new heights.
  • Improve: Make smooth-tool properly paintable
  • Improve: limitHeightChanges follows strange logic, and it's unknown if it always works, it shouldn't have to fix cell edges. -> Fixed the logic, but still breaks some corners in special cases. Removed the whole limit feature, opting for broken land highlight and manual edit.
  • Improve: Ability to re-attach broken cell edges (already done: function fixEdges, also implemented in #2195 )
  • Improve: Highlight excessive height changes/broken land in view window
  • Improve: Add label to toolstrength-slider
  • Improve: When creating new land next to existing land, seam cell edge propely
  • Improve: Painting height on empty cell (no land) or non-existing cell should create new land. Note: Fundamentally works, but some cells (e.g. -1, -17) still refuse to be visible.
  • Optimize: Check whichever is faster way to read land heights, TerrainStorage::getSumOfAlteredAndTrueHeight or through CSMWorld::LandHeightsColumn. Likely the former. If proven, do this optimization at all places possible. -> Checked, didn't find a safe way to do this optimization.
  • Extra: Plateau/flatten -tool
  • Extra: Post-process smoothing & roughening, when raise or lower painting tool is used
  • Check, and then uncomment or remove commented stuff at terrain and terrainstorage.
  • Fix changelog (no hurry with this one though, merge conflict prevents useless builds while updating this)
  • Test: Does smooth tool next to a broken cell edge calculate the average properly? (solve this by fixing bugs that make the cell edge break) -> Tested. If land edge is broken, tools also don't function properly. This isn't a problem, measures have been taken to prevent cell edge tearing.
  • Final cleanup
    -> Final review and merge?

Planning/feedback needed for:

  1. UI/UX, feedback on tools and general usability.
  2. Transient land change implementation (terrainstorage.cpp), any objections or better ideas?
  3. C++ style in general
  4. Memory management (I've tried to make it as automatic as possible). Should transient edits allocate memory dynamically, use std::vector instead of float[]?
  5. Terrain vertex selection drawing, how does it look, tips on optimization
  6. Limits and how they are implemented, currently limits are at terrainstorage (always steps of +-8 per change), and max height change is limited at command-phase (end of drag), limiting cuts extreme land edits to those that shouldn't break during mod save.
  7. Currently circle brush is the only tool with soft edges, all other tools have hard edges. Hardness/softness can both be useful at times. Should there also be a user option for brush hardness, or some other default setting? Implementing hardness/softness for custom brush could be based on the distance from the center of the custom brush, or a gaussian blur on top of selection shape (using a gaussian blurred "opacity map" to determine the tool strength in any vertex).

https://youtu.be/B1FCAOFqy-M (new video, demonstrating the features of the editmode. Note that after this video was made, if slope steepness limiting fails, it won't be applied to data any more.)
https://youtu.be/YtEL6MMvw9w (old video, bugs seen here should be fixed by now)

Work for other PR's

  • When slope steepnesses are checked during applying the edit (end of transient edit), the order of cells in std::vector could be changed to iterate through X and Y in order, instead of alphabetical sorting. Proper sorting should make automatic limiting more reliable. There are however no cases where alphabetical sorting has failed during testing. No need to fix if it's not broken.
  • Make slope steepness limiting real-time. (not in the scope of this PR)
  • Extra: Bump -tool (not in the scope of this PR)
  • Add option to turn automatic land limiting functions off. This can be useful in some situations (e.g. you want to make a
  • Add option for Discard -mode that lets one change also the cell edge vertexes. This can be useful when you want to define the very edges of the world, without connecting to
@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Sep 12, 2019

Diagram of classes and explanation why ESMTerrain::TerrainStorage was changed https://imgur.com/a/6ed3bDg

@unelsson unelsson changed the title [Discussion, don't merge] Editor: Transient land shape editing [Review phase] Editor: Transient land shape editing Sep 19, 2019
@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Sep 21, 2019

For the editor side, I think this is ready for review and testing. Feature is fully functional, and I've tested the crap out of it on Linux.

Cons: Real-time terrain limiting is not yet implemented, and it's debatable whether using forced red in vertex colors is good for signaling slopes that are too steep. These can be further developed before or after merging. There are also some terrain selection related things one could work on, but I'm trying not to touch terrain selection in this PR (any more than I already did...)

For the changes made to components/esmterrain/storage.cpp and components/esmterrain/storage.hpp, someone familiar with them should probably have a glance.

Sorry for extra CI- and appveyor builds.

@rhtucker

This comment has been minimized.

Copy link
Contributor

rhtucker commented Sep 23, 2019

Great job @unelsson! I played around with this for several minutes. Didn't get any crashes, tools seem to do what I would expect them to. Glad that we have both height drag and paint! One suggestion would be to have a hotkey switch between up and down when painting so you don't have to switch through the menu.

Only two real problems that I noticed, one major, one minor. Major one is that if you leave the terrain edit tool and then come back to it, the UI switches to the default settings, but the tool remembers what you set it to previously. Basically if you were editing with circle at 10 radius with height paint, then switch to instance editing mode by clicking on that icon, then switch back to terrain editing, the UI resets to point with 1 radius and height drag, but will actually edit with the circle at 10 radius and paint. Each of these will only be reflected by the UI once they've been changed.

The minor problem I noticed is that when changing height (at least with the drag tool) I expect the terrain to still be very smooth, just stretched up. Instead, some of the vertices don't seem to travel in a smooth arc upward and some of it turns out rather jagged, forcing me to use the smooth tool, which also changes the height somewhat. Not ideal if I'm trying to make precise adjustments. The left click feature where it highlights the vertices allows you to see this if you click before and then again after the edit. Certainly not the worst thing, but figured it was worth pointing out.

The left click to highlight radius and vertices is a pretty cool feature. Wondering what the intended use for it is though? Just to check how your edit compares to the previous morphology? Same with the green highlighting at cell edges. That's especially nice to know you've messed with another cell.

Only other suggestions would be to show the tool outline at all times, or maybe only if you hold down a certain hotkey. Although come to think of it, I'd rather have it shown at all times with a hotkey to turn it off. Might be helpful to have a second radius shown to indicate strength, but not sure how useful that would really be.

So awesome to have someone working on major editor features! Let me know if I need to clarify or post pics/videos etc.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Sep 25, 2019

Thanks for testing @rhtucker !

Did you test this in Windows btw? It's really nice to have this just ran through different platforms before merging.

Circle tool was using int for distance calculation, which resulted in rough terrain. I now changed this to float to have smoother shapes.

Also removed the deactivation code in order to hold tool options in memory while changing modes. Alternative would be to also delete all tool options from memory, but I suppose it's better to keep these in memory.

One suggestion would be to have a hotkey switch between up and down when painting so you don't have to switch through the menu.

Hotkey between modes is a good idea, but I'm cutting that out of this PR. This is a big feature already, so I'm trying not to change the code of the keyboard settings / controls right now. It's better not to touch too many places all at once, and surely hotkeys can be implemented later.

The left click to highlight radius and vertices is a pretty cool feature. Wondering what the intended use for it is though? Just to check how your edit compares to the previous morphology? Same with the green highlighting at cell edges. That's especially nice to know you've messed with another cell.

This is the terrain vertex selection. The basis for it was already implemented in already-merged terrain selection PR, but this PR kind of enables it. Currently it's just the very basics of this feature. Just being able to draw the vertex lines highlighted can be useful at times. This feature can be improved in many ways, and uses for it implemented later. You might e.g. use it for defining custom tool shapes, similar as texture editing. Copy-pasting terrain height might also be possible, but not in the scope of this PR.

Only other suggestions would be to show the tool outline at all times, or maybe only if you hold down a certain hotkey. Although come to think of it, I'd rather have it shown at all times with a hotkey to turn it off. Might be helpful to have a second radius shown to indicate strength, but not sure how useful that would really be.

Definitely! I've been thinking of drawing a circle around the editing area for all edit modes, but currently the way for drawing the selection has optimization problems, so implementing this takes a bit of thought. So, out of the scope of this PR.

@rhtucker

This comment has been minimized.

Copy link
Contributor

rhtucker commented Sep 25, 2019

Yep, this was on Windows 10.
And the rest of this all sounds good, I figured you wouldn’t want to include all of it but figured I’d just offer it all anyway for later. I’ll go ahead and test the changes tonight to see if anything else pops up.

declareDouble ("navi-wheel-factor", "Camera Zoom Sensitivity", 8).setRange(-100.0, 100.0);
declareDouble ("s-navi-sensitivity", "Secondary Camera Movement Sensitivity", 50.0).setRange(-1000.0, 1000.0);
declareSeparator();

declareDouble ("p-navi-free-sensitivity", "Free Camera Sensitivity", 1/650.).setPrecision(5).setRange(0.0, 1.0);
declareBool ("p-navi-free-invert", "Invert Free Camera Mouse Input", false);
declareDouble ("navi-free-lin-speed", "Free Camera Linear Speed", 1000.0).setRange(1.0, 10000.0);
declareDouble ("navi-free-rot-speed", "Free Camera Rotational Speed", 3.14 / 2).setRange(0.001, 6.28);
declareDouble ("navi-free-rot-speed", "Free Camera Rotational Speed", 3.14 / 2).setRange(0.001, 6.28);

This comment has been minimized.

Copy link
@elsid

elsid Oct 1, 2019

Collaborator

Might be better to use M_PI_2 or M_PI / 2 constant instead of 3.14 / 2 and 2 * M_PI - 0.001 instead of 6.28 for readability. Not sure does it important to exclude 2 * M_PI value from range.

This comment has been minimized.

Copy link
@unelsson

unelsson Oct 2, 2019

Author Contributor

Oh.. This is an automatic change done by Atom, using 3.14 or 6.28 or M_PI_2 isn't related to terrain editing. This change already got through in another commit, it does nothing but removes a few extra spaces, so I let it be there even if it makes this PR a bit more messy. I think adding an extra commit to revert those extra spaces back is unnecessary. Even if the review comment is actually worth thinking about, I won't touch the pi value here, as it's not related to this PR anyway.

components/esmterrain/storage.hpp Outdated Show resolved Hide resolved
std::string mCellId;
std::string mBrushTexture;
int mBrushSize;
int mBrushShape;

This comment has been minimized.

Copy link
@elsid

elsid Oct 4, 2019

Collaborator

Benefit from having enum is to define objects using this enum to make it obvious what possible values are. I'd suggest to use a single type BrushShape and to use it for all mBurshShape fields definitions. The enum type can be defined in a separate header in case there is some problem with includes.

This comment has been minimized.

Copy link
@unelsson

unelsson Oct 6, 2019

Author Contributor

As you mentioned, this was possible to do for brush shape. In some cases I'm using int values from Qt's menus (e.g. enum ShapeEditTool in 99e9516 )

@elsid

This comment has been minimized.

Copy link
Collaborator

elsid commented Oct 5, 2019

Not sure is it a related problem. Terrain brush is not located under the cursor, it is shifted somewhere: https://streamable.com/9mm46, https://streamable.com/szcs3.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Oct 5, 2019

Which os? The latest version? I've tested builds after almost all commits and havent had the same, but maybe something changed during latest code changes.

edit: I tested this again with the latest, but can't reproduce the shifting of the cursor. How would you reproduce this?

edit2: The coordinates for editing are set in CSVRender::TerrainShapeMode::drag, with editTerrainShapeGrid(CSMWorld::CellCoordinates::toVertexCoords().

@elsid

This comment has been minimized.

Copy link
Collaborator

elsid commented Oct 5, 2019

Which os?

Arch Linux

The latest version?

3ca12f9

How would you reproduce this?

It seems this happens with QT_AUTO_SCREEN_SCALE_FACTOR=1. When I remove this env variable it's fine.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Oct 5, 2019

It seems this happens with QT_AUTO_SCREEN_SCALE_FACTOR=1. When I remove this env variable it's fine.

Thanks, great find! I'm not sure exactly which point the glitch happens though, and I have zero knowledge of QT's environment variables. Google search gives some ideas that it might be possible to change it with qputenv https://doc.qt.io/qt-5/qtglobal.html#qputenv . I can't think of better right now than to check if it's 1, and if so, force it to something which works.

@elsid

This comment has been minimized.

Copy link
Collaborator

elsid commented Oct 5, 2019

Probably it's not only about this feature. Need to check. If it is then bug is more general one and no need to fix it by this pr.

@elsid

This comment has been minimized.

Copy link
Collaborator

elsid commented Oct 5, 2019

Same problem with terrain texture editing at master. I will file a bug for this.

@unelsson unelsson force-pushed the unelsson:transientlandshapeedit branch from 2312f08 to 6f9f59d Oct 22, 2019
#include <set>
#include <memory>

#include "../../model/world/land.hpp"
#include "../../model/world/landtexture.hpp"

#include <components/esmterrain/storage.hpp>
#include <components/debug/debuglog.hpp>
#include <components/misc/resourcehelpers.hpp>
#include <components/vfs/manager.hpp>
Comment on lines 3 to 12

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Oct 22, 2019

Collaborator

I think some of these are not necessary. Please review this list.

This comment has been minimized.

Copy link
@unelsson

unelsson Oct 22, 2019

Author Contributor

Indeed true! Artefacts from the past versions of this PR. I did go through includes, but apparently forgot this file (at this point). Fixed in fdc73b8 .

{
for(int j = 0; j < ESM::Land::LAND_SIZE; ++j)
{
if (paged)

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Oct 22, 2019

Collaborator

Don't leave the landShapeNew array values uninitialized if worldspace widget is not paged. Move paged check down to the condition with getCellAlteredHeight call.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Oct 24, 2019

Right, thank you @Capostrophic, @elsid and others for helping to review.

We should get this in.

Great job @unelsson

@psi29a psi29a merged commit 9f039fa into OpenMW:master Oct 24, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@psi29a psi29a changed the title [Review phase] Editor: Transient land shape editing Editor: Transient land shape editing Oct 24, 2019
@Capostrophic

This comment has been minimized.

Copy link
Collaborator

Capostrophic commented Oct 24, 2019

Crossing fingers.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Oct 24, 2019

And toes and eyes...

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