logo
Page Index Toggle Pages: 1 Send TopicPrint
Hot Topic (More than 10 Replies) Issue #41. Returning none from SpawnNotification may result in game crash (Read 2326 times)
Masterkent
Developer Team
Offline



Posts: 1345
Location: Russia
Joined: Apr 5th, 2013
Gender: Male
Issue #41. Returning none from SpawnNotification may result in game crash
Dec 29th, 2016 at 10:48am
Print Post  
When there is a chain of SpawnNotify actors and SpawnNotification of one SpawnNotify actor in the chain returns none, the implementation may try to choose the next SpawnNotify actor for further processing, such an attempt results in immediate game crash. A trivial example that reproduces the issue:

Code
Select All
class SpawnNotifyTest expands Mutator;

event PostBeginPlay()
{
	Spawn(class'CustomSpawnNotify');
	Spawn(class'CustomSpawnNotify');
	SetTimer(1, false);
}

event Timer()
{
	Spawn(class'DispersionAmmo');
} 


Code
Select All
class CustomSpawnNotify expands SpawnNotify;

event Actor SpawnNotification(Actor A)
{
	if (Projectile(A) != none)
		return none;
	return A;
} 


https://www.upload.ee/files/6499722/SpawnNotifyTest.u.html (run this either as a mutator SpawnNotifyTest.SpawnNotifyTest or by using "summon SpawnNotifyTest.SpawnNotifyTest").

Returning none from SpawnNotification may be useful, so I think that this should be fixed (When any SpawnNotification returns none, the corresponding Spawn function should immediately return none without invocation of the subsequent SpawnNotification functions in the chain).
  
Back to top
 
IP Logged
 
Smirftsch
Forum Administrator
*****
Offline



Posts: 8100
Location: at home
Joined: Apr 30th, 1998
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #1 - Dec 30th, 2016 at 7:19am
Print Post  
thanks, will check on this
  

Sometimes you have to lose a fight to win the war.
Back to top
WWWICQ  
IP Logged
 
han
Global Moderator
Unreal Rendering Guru
Developer Team
*****
Offline


Oldunreal member

Posts: 595
Location: Germany
Joined: Dec 10th, 2014
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #2 - Dec 30th, 2016 at 9:54pm
Print Post  
I don't see the point in returning None in the SpawnNotify. If no operations happens it should just return the passed in Actor.

Also passing the None result back to the Spawn functions itsself makes no sense as there was actually an Actor spawned, but you just didn't get a handle.

The crash itsself seems to be easy to fix, however imho it should be replaced with a voluntary crash, e.g. add some check(Actor) afterwards, and not even start with allowing SpawnNotifies to return None.
  

HX on Mod DB. Revision on Steam. Löffels on Patreon.
Back to top
 
IP Logged
 
[]KAOS[]Casey
Developer Team
Betatester
Offline


nedm

Posts: 3214
Joined: Aug 7th, 2011
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #3 - Dec 31st, 2016 at 6:38am
Print Post  
return None would be equvilant to other.destroy in checkreplacement, though? I've never actually used spawnnotify, what does UT99 do?
  
Back to top
 
IP Logged
 
han
Global Moderator
Unreal Rendering Guru
Developer Team
*****
Offline


Oldunreal member

Posts: 595
Location: Germany
Joined: Dec 10th, 2014
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #4 - Jan 1st, 2017 at 1:20am
Print Post  
No for destroying an Actor, you would simply call Destroy() on it and return it nontheless. However, in this case there is no point in using a SpawnNotify, using a Mutator is fine anyway. What SpawnNotifies actually allow is replace the object in place (should be a child class though...), so the code which called the Spawn() just works on the replaced object. You can't do this with a Mutator. Also SpawnNotifies is handled after all the *Begin() events are called, and a bit of other internal init code. So with a Mutator you can discard the Actor earlier compared to done inside a SpawnNotify.

Even in case, lets say one would allow returning None, one shouldn't implicit Destroy() the object afterwards. This wouldn't be any new thing you couldn't have done before in any way, but might allow some highjacking. However this is still questionable if this makes sense to do this, if you want to delete a newly spawned actor using a Mutator is the better place to do it.
  

HX on Mod DB. Revision on Steam. Löffels on Patreon.
Back to top
 
IP Logged
 
Masterkent
Developer Team
Offline



Posts: 1345
Location: Russia
Joined: Apr 5th, 2013
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #5 - Jan 3rd, 2017 at 7:59pm
Print Post  
han wrote on Jan 1st, 2017 at 1:20am:
No for destroying an Actor, you would simply call Destroy() on it and return it nontheless. However, in this case there is no point in using a SpawnNotify, using a Mutator is fine anyway.

It's not fine if the actor's PreBeginPlay does not let you handle the actor spawning by means of mutator's CheckReplacement.

Ideally, SpawnNotify should offer two handlers for different stages of actor construction: something like PreSpawnNotification should be called before PreBeginPlay and SpawnNotify should be called at some moment after PostBeginPlay (as it does currently).

Quote:
What SpawnNotifies actually allow is replace the object in place (should be a child class though...), so the code which called the Spawn() just works on the replaced object.

Another big advantage of SpawnNotify is that it has no restrictions related to actor's PreBeginPlay.

Quote:
Also SpawnNotifies is handled after all the *Begin() events are called, and a bit of other internal init code. So with a Mutator you can discard the Actor earlier compared to done inside a SpawnNotify.

Yes, so currently we have to choose between unreliable mutator's CheckReplacement or reliable SpawnNotify's SpawnNotification that may be invoked after the handled actor already produced some side effects.
  
Back to top
 
IP Logged
 
han
Global Moderator
Unreal Rendering Guru
Developer Team
*****
Offline


Oldunreal member

Posts: 595
Location: Germany
Joined: Dec 10th, 2014
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #6 - Jan 3rd, 2017 at 10:02pm
Print Post  
Masterkent wrote on Jan 3rd, 2017 at 7:59pm:
It's not fine if the actor's PreBeginPlay does not let you handle the actor spawning by means of mutator's CheckReplacement.

In fact it is. That behaviour is specifically used for things like Projectiles and Fragments. And using a SpawnNotify isn't a good way of handling them anyway. There is a lot of Spawning of them going around and replacing the source Actor they are spawning from in the first place, or modifying a source actors property to change the spawning class. If these changes are desired, attention should be paid to have no hardcoded class inside Spawn, but rather expose them as a property of a class defaulting to the original value.

Quote:
Ideally, SpawnNotify should offer two handlers for different stages of actor construction: something like PreSpawnNotification should be called before PreBeginPlay and SpawnNotify should be called at some moment after PostBeginPlay (as it does currently).

Eventually allowing the spawn class to be modified in the first place to a subclass might be a way to go.

Quote:
Another big advantage of SpawnNotify is that it has no restrictions related to actor's PreBeginPlay.

Which are?

Quote:
Yes, so currently we have to choose between unreliable mutator's CheckReplacement or reliable SpawnNotify's SpawnNotification that may be invoked after the handled actor already produced some side effects.

I don't see why mutors checkreplacement is unreliable? That it is limited to some classes doesn't make it per se unreliable.

Also the side effects are usually (at least for Deus Ex) caused by missing checks for bDeleteMe inside the *Begin() functions. Super's PreBeginPlay() should always be called before the own code and canceled if the Actor is already Destroyed (e.g. bDeleteMe) raised. So if these checks would be in, there wouldn't any side effects happening.
  

HX on Mod DB. Revision on Steam. Löffels on Patreon.
Back to top
 
IP Logged
 
[]KAOS[]Casey
Developer Team
Betatester
Offline


nedm

Posts: 3214
Joined: Aug 7th, 2011
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #7 - Jan 4th, 2017 at 12:03am
Print Post  
CheckReplacement doesn't work on projectiles because of bGameRelevant=true

Code
Select All
event PreBeginPlay()
{
	// Handle autodestruction if desired.
	if( !bGameRelevant && (Level.NetMode != NM_Client) && !Level.Game.IsRelevant(Self) )
		Destroy();
} 



I'm not exactly sure why this is, since you can replace essentially anything else. SpawnNotify lets you replace these, of course. Too bad it didn't exist in pre UT before 227.. making replacing projectiles impossible unless you hacked it cleverly, or in a lame way like replacing the whole pawn/weapon that created the projectile
  
Back to top
 
IP Logged
 
han
Global Moderator
Unreal Rendering Guru
Developer Team
*****
Offline


Oldunreal member

Posts: 595
Location: Germany
Joined: Dec 10th, 2014
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #8 - Jan 4th, 2017 at 7:34am
Print Post  
[]KAOS[]Casey wrote on Jan 4th, 2017 at 12:03am:
I'm not exactly sure why this is, since you can replace essentially anything else. SpawnNotify lets you replace these, of course. Too bad it didn't exist in pre UT before 227.. making replacing projectiles impossible unless you hacked it cleverly, or in a lame way like replacing the whole pawn/weapon that created the projectile

You still spawn two actors and also doing these frequently, while for Pawns, Decorations you just do this once on level startup, or infrequently. For projectiles, fragments, you would in the end all the time spawn them twice which is imho bad.

If it is cumbersome to replace projectile, the weapon code itsself should be changed so this gets easy as I suggested above. This way there is no performance impact or anything else.
  

HX on Mod DB. Revision on Steam. Löffels on Patreon.
Back to top
 
IP Logged
 
Masterkent
Developer Team
Offline



Posts: 1345
Location: Russia
Joined: Apr 5th, 2013
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #9 - Jan 4th, 2017 at 8:06am
Print Post  
han wrote on Jan 3rd, 2017 at 10:02pm:
That behaviour is specifically used for things like Projectiles and Fragments.

Some classes neither call super.PreBeginPlay nor invoke CheckReplacement directly at all. F.e., UnrealShare.Blood2.

Quote:
And using a SpawnNotify isn't a good way of handling them anyway.

Why?

Quote:
There is a lot of Spawning of them going around and replacing the source Actor they are spawning from in the first place, or modifying a source actors property to change the spawning class. If these changes are desired, attention should be paid to have no hardcoded class inside Spawn, but rather expose them as a property of a class defaulting to the original value.

You can control the spawn class only in a code that you own. I wish you good luck with trying to change a projectile's spawn class in something like UnrealShare.EightBall. Besides, changing a spawn class is not a scalable solution in case if we want to replace actors without caring about who or what spawns them.

Quote:
Quote:
Ideally, SpawnNotify should offer two handlers for different stages of actor construction: something like PreSpawnNotification should be called before PreBeginPlay and SpawnNotify should be called at some moment after PostBeginPlay (as it does currently).

Eventually allowing the spawn class to be modified in the first place to a subclass might be a way to go.

Good luck with changing all UScript code base to make it fit the "ideal" model you suggest. And then good luck with modifying the corresponding properties of all potential spawners that might ever spawn an actor of a type which is supposed to be replaced.

Quote:
Quote:
Another big advantage of SpawnNotify is that it has no restrictions related to actor's PreBeginPlay.

Which are?

Whether CheckReplacement is called depends on
1) whether our code is executed on a server (if we're on a network client, goodbye CheckReplacement);
2) whether PreBeginPlay calls CheckReplacement directly or indirectly (if not, goodbye CheckReplacement).

Quote:
I don't see why mutors checkreplacement is unreliable? That it is limited to some classes doesn't make it per se unreliable.

It _is_ unreliable if we handle an arbitrary class hierarchy with a certain base class. The hierarchy may include a subclass that does not invoke CheckReplacement in its PreBeginPlay.

Quote:
Also the side effects are usually (at least for Deus Ex) caused by missing checks for bDeleteMe inside the *Begin() functions.

In case of UnrealShare.Rocket, there is no a simple way to replace a projectile (neither with CheckReplacement nor with SpawnNotification). In case of SpawnNotify an observable side-effect (rocket spawn sound) will be produced before we enter the SpawnNotification.

Quote:
Super's PreBeginPlay() should always be called before the own code and canceled if the Actor is already Destroyed (e.g. bDeleteMe) raised.

Good luck with fixing all third-party classes that do not follow that programming pattern.
  
Back to top
 
IP Logged
 
[]KAOS[]Casey
Developer Team
Betatester
Offline


nedm

Posts: 3214
Joined: Aug 7th, 2011
Gender: Male
Re: Issue #41. Returning none from SpawnNotification may result in game crash
Reply #10 - Jan 4th, 2017 at 8:34am
Print Post  
han wrote on Jan 4th, 2017 at 7:34am:
You still spawn two actors and also doing these frequently, while for Pawns, Decorations you just do this once on level startup, or infrequently. For projectiles, fragments, you would in the end all the time spawn them twice which is imho bad.

If it is cumbersome to replace projectile, the weapon code itsself should be changed so this gets easy as I suggested above. This way there is no performance impact or anything else.



you would already spawn 2 actors for either spawnnotify or checkreplacement

projectiles aren't exactly spawned at an alarming rate unless you play on my server lol

also can't change closed source mods
  
Back to top
 
IP Logged
 
Page Index Toggle Pages: 1
Send TopicPrint
Bookmarks: del.icio.us Digg Facebook Google Google+ Linked in reddit StumbleUpon Twitter Yahoo