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

Leak: removeDecal(m_boundingBox) looks like memory leak. #62

Open m-7761 opened 5 years ago

m-7761 commented 5 years ago

I think this is the red selection box. It looks like these are not deleted.

EDITED: Instead of using new on this and m_rotatePoint it seems like it would be easier/practical to point to memory in the Tool object.

m-7761 commented 5 years ago

FWIW I can't see any need for DecalManager. The only way that it's used is a form of abuse by way of the omnipresent DecalManager::getInstance()->modelUpdated(model).

I think models already have a Model::Observer list that should better serve that role, and should work automatically if there are not holes in the implementation.

As I've implemented it the only Observer in the program is the main window object. The "Tool Parent" object is the view-panel area (not the individual views) but honestly, that's just an extension of the main window. Maybe there is no need for Tool::Parent either.

m-7761 commented 5 years ago

SelectFaceTool leaks "NormalTest" just a few lines up from the m_boundingBox leak. These don't need to be heap objects at all. The test also can be rewritten to compare the existing face-normals to the viewing plane's normal. (It's pretty heavy duty by comparison.)

m-7761 commented 5 years ago

SelectFaceTool seems to have another problem in that it doesn't always call removeDecal. I've tentatively disabled some code that looks like the following. The problem is it adds the "decal" unconditionally, but removes it conditionally. (If ModelViewport.cc prevents overlapping buttons it's not required... if it doesn't it's double-adding/failing to remove the decals.)

    //FIX ME (BROKEN)
    /*What is this? It's blocking RemoveDecal.
    if(m_unselect) //???
    {
        // We're waiting for the right button
        if(buttonState&BS_Left) return;
    }
    else
    {
        // We're waiting for the left button
        if(buttonState&BS_Right) return;
    }*/

(I think just having the tools draw themselves would probably be better off than this "decal" system.)

m-7761 commented 5 years ago

These select tools are 99% identical to one another. Maybe they didn't start out that way. But most of the functionality is implemented in the Model class. So it's just a matter of selecting the selection mode enum for the most part.

This library is apparently in serious need of boiling down. They are identical in terms of these bugs/leaks too, so it's not worth fixing the bugs more than once. I've consolidated them into 1 tool file. I did them last, so there are 19 tool files after auditing/reworking. (Removing the H files cuts the number in half, and doing away with the "widget" files that tools with parameters had, and consolidating tools accounts for the rest.)

To do this I've removed the notion that the 0th index is a submenu, and added an index to the icon argument. I just think of this system as a way to implement multiple tools with one interface. And am using the Path system to generate the submenus. That solves #57.

P.S. I don't know how crazy I am about persistent tools. Image editors often work like this. So do UI editors. 3D editors generally don't. I suspect the reason for that may have more to do with ergonomics, since 3D modeling can be very labor intensive.