coronalabs / corona

Solar2D Game Engine main repository (ex Corona SDK)
https://solar2d.com/
MIT License
2.54k stars 273 forks source link

Add property to track deleted/to be deleted display objects #646

Closed XeduR closed 1 year ago

XeduR commented 1 year ago

Calling display.remove( object ) or object:removeSelf() will not actually delete the display object until the next frame. In some rare instances, this can result in bugs and crashes if the developer attempts to perform certain operations on the objects after they've been removed (via references, for instance).

Some developers are currently checking to see if object.x or object.width are nil, for instance, to determine if a display object has been removed or not, but these properties only update the next frame after Solar2D has properly disposed of the display objects.

By including this new "private" convenience property, object._isRemoved = true, developers would always be able to know if an object has been removed or is about to be removed by next frame. Naturally, this would only affect display objects removed via display.remove, but it has long been Solar2D's recommend way of removing display objects.

Code example of the issue:

local rect = display.newRect( 0, 0, 100, 100 )

display.remove( rect )
print( rect.width ) -- 100

timer.performWithDelay( 1, function()
    print( rect.width ) -- nil
end )

This would also apply to widgets, for instance:

local widget = require( "widget" )

-- Function to handle button events
local function handleButtonEvent( event )
    if ( "ended" == event.phase ) then
        print( "Button was pressed and released" )
    end
end

-- Create the widget
local button1 = widget.newButton(
    {
        left = 100,
        top = 200,
        id = "button1",
        label = "Default",
        onEvent = handleButtonEvent
    }
)

display.remove( button1 )
print( button1._isRemoved ) -- true

This single property would remove the guesswork from knowing whether a display object in Solar2D has been removed or not.

Be1zebub commented 1 year ago

why add even more weird api? add something like: display.isValid(object) & object:isValid() like:

function display.isValid(object)
      return object ~= nil and object.x and object._isRemoved ~= true
end
XeduR commented 1 year ago

object:isValid() would crash if object is nil, since you couldn't make the method call.

display.isValid( object ) would have a confusing name as it wouldn't actually validate if a given display object wasn't invalid or corrupted. It could be called display.isRemoved( object ), but if turned into a separate API, then the object._isRemoved property would need to be included in the method call as well and your example would need to be further modified.

Be1zebub commented 1 year ago

object:isValid() would crash if object is nil, since you couldn't make the method call.

thats why display.isValid(object) needed, method its alternative way for oop enjoyers

display.isValid( object ) would have a confusing name

what confuses you? valid means - its object, its not removed, is it safe to work with this object.

as it wouldn't actually validate if a given display object wasn't invalid or corrupted. It could be called display.isRemoved( object ), but if turned into a separate API, then the object._isRemoved property would need to be included in the method call as well and your example would need to be further modified.

I mean, _isRemoved you want to add is a weird and little-functional api. display.isValid would general purpose, this would help not only in your case, but also in other cases.

you suggest using if obj and obj.x and obj._isRemoved ~= true then ... end instead, it would be better to have a good api, rather than using workarounds

XeduR commented 1 year ago

thats why display.isValid(object) needed, method its alternative way for oop enjoyers

You literally cannot have a method to do a check like this. If the display object has been removed and has been properly set to nil already, then trying to call object:isValid() when object = nil, you will encounter an immediate runtime error and the application will crash. Using display.isValid( object ) would be the only option for something like this.

what confuses you?

Like I said, checking if 1) a given input is a valid display object, or 2) a given input is/was a display object that has been removed, are two different things.

I do agree that adding said convenience function would be a better approach, but its name shouldn't be display.isValid() because it does not check for the validity of a display object nor could it do so reliably. There's no way to identify if an image's texture is corrupted at that point if it was created using display.newImageRect(), for instance. If I was going to use an API to check if an input was a valid display object, then I'd expect it to check that it's not corrupted and not just that it hasn't been removed.


I agree that a convenience API would be a cleaner approach, but it does still require the display object property to be added. It would, however, need to be inserted inside the object.removeSelf() method call so that it covers all cases. This PR simply needs to be modified and extended.