Bugfix/particle system #314

Merged
merged 2 commits into from Dec 25, 2016

Projects

None yet

2 participants

@victorlevasseur
Collaborator

By using sf::Texture::getNativeHandle().

However, it needs SFML 2.3 at least, so I've updated SFML to 2.4.1. SFML is now embedded into the repository so it's more easier to track its updates (we are sure that everybody has the good SFML version and it's also easier for build workers as they wouldn't have been able to update SFML by themselves).

In fact, I've been building GDevelop with SFML 2.3.1 for months on my computer, that's why I first fixed the bug by use a SFML >= 2.3 feature).

@4ian
Owner
4ian commented Nov 19, 2016 edited

I have to check on OS X to be sure we don't have any regression/weird crash.
In particular, I made a adhoc fix at the time in SFML to avoid a crash in the IDE (without being really sure about what is the real problem, that's why it wasn't commited on SFML).

See SFML/SFML#824

@4ian
Owner
4ian commented Nov 23, 2016 edited

@victorlevasseur Got this error (OS X) :/

[ 35%] Building CXX object Core/CMakeFiles/GDCore.dir/GDCore/Tools/HelpFileAccess.cpp.o
/Users/florian/Projects/F/GD/Core/GDCore/Tools/FileStream.cpp:62:13: error: constructor for 'gd::FileStream' must explicitly initialize the base
      class 'std::iostream' (aka 'basic_iostream<char>') which does not have a default constructor
FileStream::FileStream()
            ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/istream:1728:48: note:
      'std::__1::basic_iostream<char>' declared here
_LIBCPP_EXTERN_TEMPLATE(class _LIBCPP_TYPE_VIS basic_iostream<char>)
                                               ^
/Users/florian/Projects/F/GD/Core/GDCore/Tools/FileStream.cpp:68:7: error: no matching constructor for initialization of 'std::iostream'
      (aka 'basic_iostream<char>')
        std::iostream(),
             ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/istream:1488:14: note: candidate constructor
      not viable: requires single argument '__sb', but no arguments were provided
    explicit basic_iostream(basic_streambuf<char_type, traits_type>* __sb);
             ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/istream:1493:5: note: candidate constructor
      not viable: requires single argument '__rhs', but no arguments were provided
    basic_iostream(basic_iostream&& __rhs);
    ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/istream:1728:48: note: candidate constructor
      (the implicit copy constructor) not viable: requires 1 argument, but 0 were provided
_LIBCPP_EXTERN_TEMPLATE(class _LIBCPP_TYPE_VIS basic_iostream<char>)
                                               ^
2 errors generated.
make[2]: *** [Core/CMakeFiles/GDCore.dir/GDCore/Tools/FileStream.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
1 warning generated.
make[1]: *** [Core/CMakeFiles/GDCore.dir/all] Error 2
make: *** [all] Error 2

Maybe not related to this PR, I've not tried the latest master which may also be containing this compilation issue.

@victorlevasseur
Collaborator

Weird that GCC doesn't catch this one. I forgot to put the base class initialization that is required as iostream has no default ctor.

@4ian
Owner
4ian commented Nov 23, 2016

Weird indeed. Are you applying a fix on master? If it's the case, you may want to then git rebase this PR on master.

@victorlevasseur
Collaborator

Yes, coming soon.

@4ian
Owner
4ian commented Nov 23, 2016

Cool! :) I'm somewhat unable to compile master quickly because my SFML folder was erased and I'm not sure about the version I was using before for OS X.
Anyway let me know when you've come up with the fix.

Also concerning SFML embedding, maybe we could use git submodules? (See https://git-scm.com/docs/git-submodule). Basically SFML could be pointing to the SFML git (with a specific commit so that we're sure everyone has the same version). Also, CMake could fetch it in case the submodule has not been fetched (git clone must be called with argument --recursive if you want to automatically fetch the submodule :( So if CMake can take care of it, it would make the life easier if one forget to properly clone the git of GD).

Anyway, for now let's stick the embedded version as you did. If the PR is working properly everywhere we can always rework it to have this fancy submodule system to fetch SFML without having to include everything in the GD git. :)

@victorlevasseur
Collaborator

Yes, I considered this but the git submodule thing is kind of complicated: the default pull command don't fetch and update to the lastest pointed revision of the submodule...

@4ian
Owner
4ian commented Nov 23, 2016

Mmm yes I see.... What about updating then the CMake command that currently fetch the SFML archive by a call to git clone https://github.com/SFML/SFML.git and then git reset --hard somecommit (in the SFML directory)?
Do you think that would work? That would only need Git (already required to format the version number) and could be a good compromise: no need to include everything, no need to use complicated submodule stuff?

@victorlevasseur
Collaborator

I think we can use submodules but not yet as a custom patch is applied to SFML to make it work with GCC 5.x and 6.x (there's a bug with a not properly exported in the standard lib). So, the best idea now is just to keep SFML directly in the source code.

@victorlevasseur
Collaborator
victorlevasseur commented Dec 24, 2016 edited

Is it working on Mac OS X ?

(By the way, can you restart the Windows server?)

@4ian
Owner
4ian commented Dec 24, 2016

I'm cloning this branch in a fresh repo and testing it :)
Latest SFML version should work as I'm using a recent one on OS X, but with a tiny fix/workaround (see SFML/SFML#824).

I've restarted the server.

@4ian
Owner
4ian commented Dec 24, 2016

@victorlevasseur It's working OK after applying the fix of my last post.
I'm still a bit reluctant to include the whole SFML lib in the GD git.. Using git to fetch it and apply fixes would be cleaner I think, and it would self-document the custom fixes that are needed (until they are merged into SFML).

@victorlevasseur
Collaborator

Ok, I'll definitely try to improve this. If Git is not available, the script will ask the user to self download SFML and apply the fix to it.

@victorlevasseur
Collaborator

Can I get the Mac OS X fix as a git patch?

@victorlevasseur victorlevasseur Update to SFML 2.4.1 and fix particle texturing
d7509cf
@victorlevasseur
Collaborator

Done. You can add your git patch in ExtLibs/SFML-patches and it will be applied by CMake.

@4ian
Owner
4ian commented Dec 25, 2016

This looks super nice! 💖
Here is the patch for OS X (sorry, GitHub don't want to attach .patch file, I had to zip it):
02_workaround_OSX_crash_SFViewController.patch.zip

Not sure if I can commit directly to your branch? Anyway feel free to add it.
Once it's done I guess we can do a final quick check and merge this. 😊 I really like the git+patch way to get SFML, it's super clean and lightweight. Good job!

@victorlevasseur
Collaborator

Added. The only downside of the current system is that it requires git, an internet connection (I need to update the OBS script to git clone SFML into the package sent to their server) and each time CMake is executed, it will rebuild parts of SFML and redo the linking on all GDevelop targets.

@victorlevasseur victorlevasseur Add the Mac OS X workaround for SFML
Fix PackageForOBS to embed SFML in the source archive
4e32b64
@victorlevasseur
Collaborator

Done.

@victorlevasseur
Collaborator

Can I merge it?

@4ian
Owner
4ian commented Dec 25, 2016

Let's do this! :)

@4ian 4ian merged commit 65b02a7 into 4ian:master Dec 25, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment