zturtleman / mm3d

Maverick Model 3D is a 3D model editor and animator for games.
https://clover.moe/mm3d
GNU General Public License v2.0
115 stars 23 forks source link

Many (most?) model methods neglect m_changeBits #90

Open m-7761 opened 4 years ago

m-7761 commented 4 years ago

This is why DecalManager::getInstance is abused to force redraw.

I'm just going to go down the files and slip them in as best I can.

P.S. I'm removing DecalManager ATM. It's an abomination in terms of its STL maps that are all over the place, and I'm pretty sure incorrect. They are tangled up like a ball of Christmas tree lights. In terms of replacing its functionality it's as simple as adding a draw method to Tool's interfaces.

m-7761 commented 4 years ago

Update:

I found the following don't change m_changeBits.

    bool unselectAllVertices();
    bool unselectAllTriangles();
    bool unselectAllGroups();
    bool unselectAllBoneJoints();
    bool unselectAllPoints();
    bool unselectAllProjections();

I ended up having each do beginSelectionDifference() at the top. It's a no-op if already called... but operationComplete() calls endSelectionDifference(). I think it needs more attention, but that should fix them.

Otherwise, I found that moveVertex does indirectly set m_changeBits through invalidateNormals, but I'm not sure that's intentional. Many others do so intentionally. movePoint doesn't seem to. And moveVertex also calls setFrameAnimVertexCoords on the animation-mode path... which doesn't seem to either.

The real problem is dialog boxes that make model changes prior to OK can't do operationComplete, so need another way to update the view. Probably the closest to that is to call updateObservers directly. While not so obvious, it's much more correct than to abuse DecalManager. Neither is the cancel API so obvious: undoCurrent.

undoCurrent does updateObservers itself, so I think it would be sound to replace DecalManager with updateObservers... if all of Model's APIs set m_changeBits correctly. Unfortunately that's hard to demonstrate, but I think it best to make the change and debug the problems as they arise.

I may add a default parameter to updateObservers to try to force redrawing, to make it at least as good as the current situation. There's likely a multitude of APIs that neglect to modify m_changeBits, but it would need a full audit of the Model code to say for certain.

m-7761 commented 4 years ago

To be able to use "getUndoEnabled" the following lines need to be reversed, as do the ones in the "redo" method.

https://github.com/zturtleman/mm3d/blob/dcb15f3440a62376ed2e09d01960b58426c13ef6/src/libmm3d/model.cc#L2380-L2382

I don't believe "observers" should be making modifications in response to change-notices, since undo/redo is for end-user interactions.

Apropos setTextureCoords doesn't set m_changeBits. I guess UVs is "MoveGeometry" if normals are.

P.S. Today I dug into texturecoord.cc. I wanted to add a Properties panel to it so you can at least get coordinate values out of it. I found it doesn't require its data to function since all of the information is in the TextureWidget object. The "Group" function uses a map to avoid duplicating vertices, but the function that initializes the UV map does not. It doesn't matter since you can't select individual UV pairs. I think there's probably room for simplification. The practice of mapping to addTriangle and addVertex makes no sense since they always just push elements onto the back of their buffer. It's programmed way too defensively. It makes it look like everything is way more complicated than it is. Which is bad. Combined with the Qt spaghetti coding it's hard to maintain. I think my UI work has merit not just for being better than the current Qt UI (which I assume can be improved) but it's also way more practical and legible. So I think it should be considered. Anyway, porting my changes to Qt would be more work than keeping the UI code I've developed, since the "implui" folder code is married to the UI framework. But it will need a lot more testing on Linux/Apple systems and probably another round of enhancements to make it it a seamless transition.

m-7761 commented 4 years ago

None of the specific selection subtypes (e.g. SelectionFaces) are set. I just went through my model_select.cc file and added tests to set them only if a primitive was not selected prior to selection and vice-versa, and also delayed setting until endSelectionDifference so that it can be specific. IOW, I've made it so the change-notices reflect the actual changes, and not just that a tool was used.

I removed SelectionChange from setSelectedUv since nothing can see UV selection changes as long as they are limited to the UV editor.

I'm trying to not make changes to the Model parts of the library, but nothing works without change-notices, so it's impossible to ignore. The UI element works much better if the main window is the only "observer" and change-notices are in place. I think the existing organization must take its cue from Qt (I don't know if it was always Qt based) but how it encourages linking things up willy-nilly is really the worst possible thing to do, since human-being (programmers) are already tool willy-nilly for our own good.

EDITED: This may have gummed things up. I can't say... MU_Select is pathalogical because it uses the generic unselect functions in a loop, some of which have crazy side-effects like iterating over every triangle and setting vertices selected for some reason. I.e. another model-wide loop within loop. (I'm sure if vertices had quarks or something there would be a loop within a loop within a loop lurking somewhere.)

On the plus side, I'm not seeing the glitch with the Cylinder tool's surface-normals. On the downside, it seems very clunky, and undos are slow to unresponsive. I fear installing change-notices may be making feedback loops or something. But also, for some reason software is often written as-if computers are magic: that is, complete disregard for what is performant/can fit into memory. MM3D's principal author (Kevin?) must have shared this proclivity.

m-7761 commented 4 years ago

NOTE: The Model::ChangeBits enum values have to be reassigned so they don't have bogus values (a number of them have the same bit/value.)

m-7761 commented 4 years ago

For the record, a little while back I did a much more extensive round of work on this problem to support various values in the Properties sidebar. I won't say it's complete, but I think it's nearly so if not so.

I'm going to publish the code omnibus sometime in the next weeks.

m-7761 commented 4 years ago

Solved: #134

m-7761 commented 4 years ago

mergeAnimations in viewwin.cc neglects to call operationComplete to enable undo and flush change-notices.

zturtleman commented 4 years ago

Merge animations undo event is fixed by fb66ccb21f772a2f7f168880dced412c50547fb4. Thanks.