search

mickelson / attract

A graphical front-end for command line emulators that hides the underlying operating system and is intended to be controlled with a joystick or gamepad.
http://attractmode.org
GNU General Public License v3.0
393 stars 115 forks source link

For discussion: alternative implementation of PR #509 #524

Closed mickelson closed 5 years ago

mickelson commented 5 years ago
oomek commented 5 years ago

Thanks for refactoring. It look cleaner, although I believe In certain situations it may have a negative impact. When you have at least one SWF element on the screen it's fine, but imagine the following scenario:

Your currently selected game has a SWF element, then you navigate to a game which has only images or mp4s, swf context gets destroyed, then it's recreated again for the selection with another SWF element. Recreating context is a very expensive operation, so it should be initialized once when you start Attract Mode and it should live until you exit Attract Mode. Maybe you could move all the context related initialization to a separate static function and call it somewhere in present?

I can also see you kept glPushAttrib( GL_ALL_ATTRIB_BITS ); It's not needed and it's throwing GL_STACK_OVERFLOW

oomek commented 5 years ago

By saying all context related initialization I meant also all opengl code as well like I did in my original commit. Those gl lines are required to be called only once per context. FYI when you call for example glOrtho more than once in a shared context you will get flipped swf horizontally vertically.

oomek commented 5 years ago

I've tested it and I can confirm that your refactoring is causing an additional lag, comparing to my original PR. It's faster if you have many swf elements compared to the build without my original PR, but when you have just one swf there is no speed improvement.

mickelson commented 5 years ago

ok good point, I've changed it now so the sf::Context is only initialized once the first time swf gets used

oomek commented 5 years ago

Is there any particular reason you have decided to keep glPushAttrib( GL_ALL_ATTRIB_BITS ); ?

mickelson commented 5 years ago

nope, missed it, i’ll fix

On Dec 21, 2018, at 5:48 PM, Radek Dutkiewicz notifications@github.com wrote:

Is there any particular reason you have decided to keep glPushAttrib( GL_ALL_ATTRIB_BITS ); ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

mickelson commented 5 years ago

You have missed that part

attract/src/fe_image.cpp

Line 747 in e4580b7 if (( play_movies ) && ( m_swf ) && !( m_video_flags & VF_NoAutoStart ))

so the swf reacts to flag NoAutoStart

Ok I've got a fix for this as well I'll put it in with a separate commit