Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move 3rdparty dependencies to 3rdparty (part 1) #1595

Merged
merged 3 commits into from
Mar 20, 2016
Merged

Move 3rdparty dependencies to 3rdparty (part 1) #1595

merged 3 commits into from
Mar 20, 2016

Conversation

AniLeo
Copy link
Member

@AniLeo AniLeo commented Mar 18, 2016

For the sake of a more organized repository, I've moved some dependencies to a new directory for external 3rdparty dependencies (/3rdparty)

  • Move /GL to /3rdparty/GL
  • Move /OpenAL to /3rdparty/OpenAL
  • Move /minidx12 to /3rdparty/minidx12
  • Move /stblib to /3rdparty/stblib
  • Move /ffmpeg to /3rdparty/ffmpeg
  • Move /minidx9 to /3rdparty/minidx9
  • Move /glm to /3rdparty/glm
  • Move /GSL to /3rdparty/GSL
  • Move /libpng to /3rdparty/libpng

@tambry
Copy link
Contributor

tambry commented Mar 18, 2016

I think calling the folder "3rdparty" would make more sense.
And squishing the move commits together along with the fix for includes would be better, so we there wouldn't be any non-working commits, while bisecting.

@AniLeo
Copy link
Member Author

AniLeo commented Mar 18, 2016

@tambry I had a look at a few other projects, I found some with a folder for that purpose being named "3rdparty" and others with "external" or "externals".
I find externals more aesthetic than 3rdparty, but I could change it to 3rdparty if needed.

Edit1: Let me see if the buildbots build before rebasing the commits

@AniLeo AniLeo changed the title Move 3rdparty dependencies to an externals directory (part 1) Move external 3rdparty dependencies to an externals directory (part 1) Mar 18, 2016
@AniLeo
Copy link
Member Author

AniLeo commented Mar 18, 2016

Commits squashed, build bots are working;
Ready to be merged.

@AniLeo
Copy link
Member Author

AniLeo commented Mar 18, 2016

Rebased

@Nekotekina
Copy link
Member

Does it compile without DX12 SDK?

@AniLeo
Copy link
Member Author

AniLeo commented Mar 18, 2016

@Nekotekina I doubt it, since it requires including some of those files.
I was wondering if the minidx9 was actually needed though.
I can try giving it a shot, but then probably the includes in the D3D12GSRender would complain about not finding the DX12 stuff (I'm on Windows 7, so I don't think I have any DX12 libs on the system)

@vlj
Copy link
Contributor

vlj commented Mar 18, 2016

minidx9 is needed for XAudio2 despite being named "dx9".

@@ -6,7 +6,7 @@
#ifdef _WIN32
#include <Windows.h>
#include "GL/gl.h"
#include "GL/glext.h"
#include "externals/GL/glext.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to include "externals" directory so that #include <GL/glext.h> works ?
I prefer the <> syntax for non local includes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(plus "externals" is likely to be removed by linux packager to use system wise includes)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

@AniLeo
Copy link
Member Author

AniLeo commented Mar 19, 2016

I should probably add the stblibs as included dirs as well

@vlj
Copy link
Contributor

vlj commented Mar 19, 2016

Yes, and for openal too.
The rule is that c++ code should not care about the location of external include so that if you remove externals repo it still builds if the include directory are passed as command line argument.

@MrSapps
Copy link

MrSapps commented Mar 19, 2016

I think 3rdParty also makes more sense, since externals could mean the shader decompiler project which isn't 3rdParty. 3rdParty is clear that the stuff in there isn't anything to do with rpcs3.

@AniLeo
Copy link
Member Author

AniLeo commented Mar 19, 2016

Yeah, I'll change it then (although I wasn't thinking of including the shader decompiler in the externals)

@AniLeo
Copy link
Member Author

AniLeo commented Mar 19, 2016

Ok, I think it's properly done now.
I will rebase once it's ready to merge

@AniLeo AniLeo changed the title Move external 3rdparty dependencies to an externals directory (part 1) Move 3rdparty dependencies to 3rdparty (part 1) Mar 19, 2016
@AniLeo
Copy link
Member Author

AniLeo commented Mar 19, 2016

Rebased and squashed

@danilaml
Copy link
Contributor

@vlj "minidx9 is needed for XAudio2 despite being named "dx9"."
We should probably strip it down even more in the fture and leave only what's needed, like dolphin does: https://github.com/dolphin-emu/dolphin/tree/master/Externals/XAudio2_7

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

Shouldn't be merged yet, corrected some paths, will commit soon with the ffmpeg and minidx9 moving

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

I don't wanna tackle with the recompilers (asmjit/llvm) and with wxWidgets as for now, so this is basically ready for review and merge.

@vlj
Copy link
Contributor

vlj commented Mar 20, 2016

There's an issue with libpng on Travis.
No worries about llvm/asmjit, modifying build system is tedious since it requires to wait Travis and appveyor build to be triggered and complete so it's OK to do it step by step.

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

I'm on mobile but I tried applying a fix for the build bots. Will look into it tomorrow if it still doesn't build.

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

I found the problem. A typo in CMakeLists

@Nekotekina
Copy link
Member

Who suggested to squash? Now I can't see all the changes.

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

@Nekotekina I just squashed to remove some useless commits in between. Everything I did today is in the latest commit

@Nekotekina
Copy link
Member

Also not sure about glm. It was confirmed that we don't need it.

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

@Nekotekina but it's included in Emu\RSX\GL\gl_helpers.h

#include <glm/glm.hpp>
#include <glm/gtc/type_ptr.hpp>

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

For some reason the PR only shows 10 changed lines, but I changed more than that. Also, VS project files are not appearing, although they are changed. Weird.

Let me rebase this properly. -- Rebased

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

@vlj Can you merge this one first? I'm not managing to git pull --rebase upstream master this properly as it has weird merge conflicts

@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

Hm, how can I properly solve conflicts between this branch and the base branch?
I'm having a hard time trying to rebase

@danilaml
Copy link
Contributor

@MyaniPT what are these problems exactly?

Moves GL, minidx12, OpenAL, stblib to 3rdparty

Fixes AppVeyor and CMakeLists (travis-ci.yml doesn't need any changes)

Points directories in the VS solution files to the new ones on the
externals directory

Includes stuff with the < > syntax instead of " "
@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

I'm rebasing this properly

Moves glm and GSL to 3rdparty
Moves libpng to 3rdparty
@AniLeo
Copy link
Member Author

AniLeo commented Mar 20, 2016

Rebased
(Last commit fixes rpcs3_default.props that was conflicting)

vlj added a commit that referenced this pull request Mar 20, 2016
Move 3rdparty dependencies to 3rdparty (part 1)
@vlj vlj merged commit 6fab5a8 into RPCS3:master Mar 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants