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

Pandora and OpenGLES renderer.The PANDORA define is used for Pandora spe... #280

Closed
wants to merge 1 commit into from

Conversation

ptitSeb
Copy link

@ptitSeb ptitSeb commented Sep 2, 2013

This branch s compatible OpenPandora (ARM & GLES).

The PANDORA define is used for specific OpenPandora options (like screenres or keymapping)
The ARM define is used to avoid Bus Error on Arm (access of float not aligned on DWORD)
The HAVE_GLES define is used to change GL1 renderer in GLES renderer

Fixes #255.

…specific option (like screenres or keymapping)The ARM define is used to avoid Bus Error on Arm (access of float not aligned on DWORD)The HAVE_GLES define is used to change GL1 renderer in GLES rendererFixes ForsakenX#255.
@chino
Copy link
Member

chino commented Sep 2, 2013

Wow this is great!

But .. we have a bit of cleaning up to do here before we're ready to merge back..

Breaking up commits

We need to break this up into multiple smaller commits with clear commit messages for each change (please see commit message formatting https://github.com/ForsakenX/forsaken/wiki/Contributing).

You can break up commits using git rebase -i but if your not familiar with this concept yet then you can always create a test branch for playing around:

git checkout PandoraGLES &&
git checkout -b testing &&
git rebase -i

You wouldn't really lose your changes even if you did rebase on your current branch since any changes you make would create new commit trees. Your existing commit will always be af5ffb3bcca2f5817903630f5eaf9717d090a238 until you run git gc which may delete it. If you forget your commit id you can always get it back by checking the git reflog which remembers a limited number of previous head states. To throw away changes and go back to your old state just use:

git checkout PandoraGLES &&
git reset --hard af5ffb3bcca2f5817903630f5eaf9717d090a238

Ultimately the commit should only contain the required changes for the specific feature.

Splitting your commits up is really easy and the git book does a pretty good job of explaining it:

http://git-scm.com/book/en/Git-Tools-Rewriting-History#Splitting-a-Commit

Note that since you already pushed your branch up you may need to use the --force option to repush it after the changes which is fine..

Feature Branches

Note: For now we don't really need to do this since it might be a lot at first so lets just focus on breaking the commits up into multiple ones as stated above.

I think it would be smart here too maybe create multiple pull requests for each feature set (GLES, ARM, Pandora specific)/

You can easily have multiple feature branches for each specific feature set that you merge into your own master branch locally:

  • upstream -> tracks official upstream
  • feature branches -> for each pull request
  • master -> tracks main + merges in your feature branches

I think this does a pretty good job of boot strapping someone with the git concepts https://github.com/ForsakenX/forsaken/wiki/Contributing

Github also has a ton of information on clean pull requests but I think our Contributing link above does a good job of summarizing them:

https://help.github.com/articles/fork-a-repo

  • Note on this link how they name the origin repo upstream and explain how to pull in updated changes.
  • They also talk about creating local feature branches.

https://help.github.com/articles/using-pull-requests
https://github.com/blog/712-pull-requests-2-0

Remove Testing Code

There is a ton of things in there that appear to have been for your convenience / testing like printf statements and changes to the player config and Makefile that only make sense for you.

If anything convert the printf's to use DebugPrintf and Msg (MessageBox) where appropriate.

MakeFile

The Makefile changes for instance will break many other systems and need to be very carefully tested everywhere.

Updated with our changes ?

I'm curious too if you've rebased our recent Makefile and other changes?

Please also use pull --rebase not just normal pull since that would create a merge on top of your changes that you'd have to push back and that would be nasty..

OpenGLES / SDL

Is eglport something that's used for Pandora ? If it's an external dependency then maybe it should be supplied with your build system or moved to be part of forsaken-libs ..

Also does sdl not support a GLES backend? This really seems like platform/gl level details in main_sdl.c for instance that sdl should be handling for us.. Appears that Pandora has some support for SDL http://pandorawiki.org/SDL ?

ARM / Platform Sizes

I see a ton of memcpy uses you added and I'm wondering if that's simply related to architecture type lengths? If so we just need to properly handle types on ARM and then the extra code wouldn't be needed and I'm not exactly sure it's the best approach to leave it like this if ultimately the types are wrong.. We had similar problems when we moved to 64bit but we got things working by cleaning up the assumptions forsaken had about type lengths at key points .. The files are always going to be encoded in 32bit lengths since they already exist so we just have to properly handle that.. There is also many versions of Pandora consoles so do you have any of the other platforms for testing too?

User specific settings

There is some changes I see to input_sdl that I wonder if we should move to simply be key bindings that a user specifies in their personal config ?

Many changes to main_sdl.c seem to be things that you can easily enable/disable from config files and some of them are off by default so I question need to add extra defines to comment things out? I guess it can be good to disable some features if they are ultimately going to break the user's setup on that particular device..

File Paths

As far as this change ptitSeb@af5ffb3#L17R173 that's already done implicitly on non Win32 systems in util.c https://github.com/ForsakenX/forsaken/blob/master/util.c#L34 and called in https://github.com/ForsakenX/forsaken/blob/master/file.c

Summary:

  • We need to break this all up into multiple commits with clean messages (please see commit message formatting https://github.com/ForsakenX/forsaken/wiki/Contributing)
  • I would love to get rid of a ton of these memcpy's if possible (or encapsulate the concept into a macro maybe?)
  • Remove any white space type changes that aren't part of logical change
  • File name handling should be done in file.c already?
  • Player / Makefile / Input changes seem like player level settings they can control in their biker config ?
  • If things like supporting 800x680 make sense in generally then we should just add those into forsaken instead of having special defines wrapping logic for one system only

I don't mind helping here to show you how we can easily in a few minutes break this up into a bunch of smaller commits and work together on importing these changes cleanly if you like to work together ? We can either pull from each other's forks or create another fork that we both have access to so we can both push to it..

I think this is exciting that were so close to get ARM, GLES, Pandora support here !

@ptitSeb
Copy link
Author

ptitSeb commented Sep 2, 2013

Ok, I'll do a short anwser now, because it's late and I need to go sleep, I
'll do a lenghtier one tomorrow :)

About the split... well, hum, ok. I'm a trully begginer in git, so, if you
say it's fast and easy, I believe you, but right now, I have now idea on
how to it.

About the changes. SDL 1 doesn't really support GLES1. SDL2 does. For SDL1,
I use eglport to create the GLES context. It's conveniant, and can be used
on other GLES compliant architecture, like RaspberryPI or ODROID. It should
be part of the GLES port.

All the memcpy are needed because of the ARM achnitecture. Like the old
time of the 68000, certain data needs certain alignment. here, float need
to be on DWORD alignement. Because the data read are packed and are of
different size, it may happens the a float is read from an WORD aligned
offset, create a "bus error" on the ARM architecture. So I used memcpy to
avoid float access, and avoiding bus error. Without the memcpy, the game
just give a bus error when it start reading datas. Anyway, the memcpy works
on x86 architecture, if you don't like the #ifdef ARM, it can be the
default behavour if you prefer.

About some printf, yes, maybe I have forgotten a few, that can be removed
safely.

The file name messing was a bit of experiment I left, when I was wondering
why I was missing texture. It turned out I was out of video memory, so that
piece of code is probably useless too.

Most Makefile altering are important for Pandora build, but maybe I should
try to IF them.

Screen resolution and keymaping is quite important for the Pandora. Because
Pandora has some "console like" controls, it is important to have a default
controls scheme that is adapted from 1st start. That look like conveniance,
but Pandora is part console, so default config should work. There is an
exeption to that, with the Shoulder button (RSHIFT & RCTRL) that simulate
Mouse button, and is mandatory to play with the Pandora (technicaly, mouse
button are normaly simulated with a nub, but I configure that nub to be a
Joystick, so I need the mouse button somewhere else, and the system doesn't
provide any solution for that, so I have to make the game simulate the
buttons).

Sebastien.

2013/9/2 Daniel Aquino [email protected]

Wow this is great!

But .. we have a bit of cleaning up to do here before we're ready to merge
back..

We need to break this up into multiple smaller commits with clear commit
messages for each change.

I think it would be smart here too to create two pull requests one that
merges in GLES support and another that merges in ARM support. Perhaps then
a third that will have Pandora specific tweaks that are required.

You can easily have multiple feature branches for each specific feature
set that you merge into your own master branch locally:

master -> tracks main
feature branches -> for each pull request
my_master -> tracks main + merges in your feature branches

You can create a clone of your branch by switching to your branch and
running git checkout -b $new_name then use git rebase -i to split this
all up into separate commits with clean commit messages.

There is a ton of things in there that appear to have been for your
convenience / testing like printf statements and changes to the player
config and Makefile that only make sense for you.

The Makefile changes for instance will break many other systems and need
to be very carefully tested everywhere.

I'm curious too if you've rebased our recent Makefile and other changes?

Ultimately the commit should only contain the required changes for the
specific feature.

Is eglport something that's used for Pandora ? If it's an external
dependency then maybe it should be supplied with your build system or moved
to be part of forsaken-libs ..

Also does sdl not support a GLES backend? This really seems like
platform/gl level details in main_sdl.c for instance that sdl should be
handling for us.. Appears that Pandora has some support for SDL
http://pandorawiki.org/SDL ?

I see a ton of memcpy uses you added and I'm wondering if that's simply
related to architecture type lengths? If so we just need to properly handle
types on ARM and then the extra code wouldn't be needed and I'm not exactly
sure it's the best approach to leave it like this if ultimately the types
are wrong.. We had similar problems when we moved to 64bit but we got
things working by cleaning up the assumptions forsaken had at about type
lengths at key points .. The files are always going to be encoded in 32bit
lengths since they already exist so we just have to properly handle that..
There is also many types of Pandora consoles so do you have any of the
other platforms for testing too?

There is some changes I see to input_sdl that I wonder if we should move
to simply be key bindings that a user specifies in their personal config ?

Many changes to main_sdl.c seem to be things that you can easily
enable/disable from config files and some of them are off by default so I
question need to add extra defines to comment things out? I guess it can be
good to disable some features if they are ultimately going to break the
user's setup on that particular device..

As far as this change ptitSeb/forsaken@af5ffb3#L17R173ptitSeb@af5ffb3#L17R173that's already done implicitly on non Win32 systems in file.c

So in summary:

  • We need to break this all up into multiple commits with clean
    messages (please see commit message formatting
    https://github.com/ForsakenX/forsaken/wiki/Contributing)
  • I would love to get rid of a ton of these memcpy's if possible (or
    encapsulate the concept into a macro maybe?)
  • Remove any white space type changes that aren't part of logical
    change
  • File name handling should be done in file.c already?
  • Player / Makefile / Input changes seem like player level settings
    they can control in their biker config ?
  • If things like supporting 800x680 make sense in generally then we
    should just add those into forsaken instead of having special defines
    wrapping logic for one system only

I don't mind helping here to show you how we can easily in a few minutes
break this up into a bunch of smaller commits and work together on
importing these changes cleanly if you like to work together ? We can
either pull from each other's forks or create another fork that we both
have access to so we can both push to it..

I think this is exciting that were so close to get ARM, GLES, Pandora
support here !


Reply to this email directly or view it on GitHubhttps://github.com//pull/280#issuecomment-23677121
.

@chino
Copy link
Member

chino commented Sep 2, 2013

Hey there! So I updated my previous message above in place so if you check out the updated version on the web it I added steps that can walk you through using git.

Git

Git really is an awesome tool that once you start to learn how to play around with branches and rebasing will lead to some awesome moments..

Help

As stated earlier I don't mind helping out in a shared repo or just walking you through how to use git to clean this up :] !

SDL

We do have some preliminary SDL 2 support in there but there is some pending issues with key mappings and so far many other projects haven't switched away yet since good sdl2 builds aren't available or perfected on the countless distributions and desktops out there..

EGL

I'm totally down with using eglport just a bit confused on if we should include it directly into Forsaken's source or make it part of forsaken-libs package since the system should provide an implementation of it?

Raspberry PI and ODROID support would be really awesome too.. Have you had experience porting to them? I think for me the cost has held me back from buying all of these platforms :\

ARM

If the memcpy's are absolutely required then I would I imagine rather package up a nice define that we use everywhere. Does this mean there may be tons of places in the code base that require this change?

Much of the memcpy changes appeared to be in asset loading? There is some backlog tickets where I think it would be lovely to port to a json/lua format or some standard formats so we can replace all this nasty buggy asset loading code but it would be a bit of work to do :\

Makefile

I'm fine with helping to make the changes to the Makefile and helping to integrate them in a nice way and I can also handle confirming that it builds for osx, linux, win32 properly with the new changes.

Inputs

I totally agree with your opinion on making some changes to the default pilot setup.

Maybe we can clean it up a bit though so we have a clear location to add separate default configs for each type of Pandora and other systems.

The input system would be a huge rewrite though so I have been following it's convention that comes from DirectX's DirectInput where they have a single keycode for all devices. In the end that is a rather elegant solution but just requires that we map buttons like shift pads to mouse buttons if needed. Although I think we can simply move that to the default player config instead of needing special support in inputs_sdl.c but that's just at first glance so I'm open to it if it's really required.

Video

I'm fine with adding 640x480 to the default list and perhaps other changes in so we can support Pandora and other devices in a more elegant way.

Cleanup

As far as unwanted changes I can show you how to remove those once your ready or see my notes earlier on how to edit a commit after it's been pushed.

@ptitSeb
Copy link
Author

ptitSeb commented Sep 3, 2013

About Git, ok, I'll start tonight.

the "eglport.c" is mandatory for GLES, So, it can be include in the base
file, guarded with a #ifdef HAVE_GLES, or include conditionnally in the
Makefile. Your call...

The memcpy where absolutly needed yes, I think it's need by the NEON part,
that can't access non 4 bytes alligned data. Neon code is automaticaly
generated by the compiler when it found trivial memory access to optimize,
like two concecutive a.x=_pfloat++; a.y=_pfloat++; => this can be optmized
by loading both float and storing both float (wich are both concecutive in
memory) using a 64bits neon register. But that "bus error" if
pfloat&0x04!=0x04... A #define macro, why not, but 2 would be better, one
for individual value, and one for 3D vector (but just one will work of
course).

For information, here are 2 video of this port running on the Pandora:
http://www.youtube.com/watch?v=xCWIt9K_ET8 and
http://www.youtube.com/watch?v=NXzp-_ShOgE
As you can see, it's working very well already (the capture is taken from
the TVOut of the Pandora). I openned a thread in the official forum here:
http://boards.openpandora.org/index.php/topic/14178-projectx-forsaken

Sebastien.

2013/9/2 Daniel Aquino [email protected]

Hey there! So I updated my previous message above in place so if you check
out the updated version on the web it I added steps that can walk you
through using git.
Git

Git really is an awesome tool that once you start to learn how to play
around with branches and rebasing will lead to some awesome moments..
Help

As stated earlier I don't mind helping out in a shared repo or just
walking you through how to use git to clean this up :] !
SDL

We do have some preliminary SDL 2 support in there but there is some
pending issues with key mappings and so far many other projects haven't
switched away yet since good sdl2 builds aren't available or perfected on
the countless distributions and desktops out there..
EGL

I'm totally down with using eglport just a bit confused on if we should
include it directly into Forsaken's source or make it part of forsaken-libs
package since the system should provide an implementation of it?

Raspberry PI and ODROID support would be really awesome too.. Have you had
experience porting to them? I think for me the cost has held me back from
buying all of these platforms :
ARM

If the memcpy's are absolutely required then I would I imagine rather
package up a nice define that we use everywhere. Does this mean there may
be tons of places in the code base that require this change?

Much of the memcpy changes appeared to be in asset loading? There is some
backlog tickets where I think it would be lovely to port to a json/lua
format or some standard formats so we can replace all this nasty buggy
asset loading code but it would be a bit of work to do :
Makefile

I'm fine with helping to make the changes to the Makefile and helping to
integrate them in a nice way and I can also handle confirming that it
builds for osx, linux, win32 properly with the new changes.
Inputs

I totally agree with your opinion on making some changes to the default
pilot setup.

Maybe we can clean it up a bit though so we have a clear location to add
separate default configs for each type of Pandora and other systems.

The input system would be a huge rewrite though so I have been following
it's convention that comes from DirectX's DirectInput where they have a
single keycode for all devices. In the end that is a rather elegant
solution but just requires that we map buttons like shift pads to mouse
buttons if needed. Although I think we can simply move that to the default
player config instead of needing special support in inputs_sdl.c but that's
just at first glance so I'm open to it if it's really required.
Video

I'm fine with adding 640x480 to the default list and perhaps other changes
in so we can support Pandora and other devices in a more elegant way.
Cleanup

As far as unwanted changes I can show you how to remove those once your
ready or see my notes earlier on how to edit a commit after it's been
pushed.


Reply to this email directly or view it on GitHubhttps://github.com//pull/280#issuecomment-23679504
.

@chino
Copy link
Member

chino commented Sep 3, 2013

I actually commented in the forums ages ago but never moved forward with it since I never purchased one my self.

Is there anyway to simply turn off those optimizations that require the memcpy changes? I only ask this because all the memcpy changes were in Load functions which load asset files using a file pointer to read data from the disk. Forsaken files are rather small so load times are already fast. Is all the extra changes really warranted for non critical code? If this code was part of the render loops and the game was running under 100 fps then it would make sense. Forsaken on a modern system runs at 100's to even 1000's of frames per second which is massively more than the game was even designed to run at.

Are you also compiling with optimizations enabled? On the pc we actually release builds with full debug and profiling support since Forsaken was notoriously buggy in release mode (still a work in progress) and it's nice for users to be able to profile / attach a debugger in case something goes wrong..

@chino
Copy link
Member

chino commented Sep 4, 2013

Oh btw do you mind updating the link to http://forsakenx.github.io/ on the pandora site? I fear the fly.thruhere.net domain may one day disappear as dyndns is becoming more commercialized..

@ptitSeb
Copy link
Author

ptitSeb commented Sep 4, 2013

I'll update the link, but on the repo, I need to update a new package for that. So I'll prepary the PXML and will update latter. I was planning to update once pull request was merge (and no one find a "bus error" or "segmentation fault" in later level), and also update the version number.
btw, what's the current version, 1.0.0?

About compile optim, yes, I compile with optimisation (because a Pandora is less powerfull than nowaday PC, it need optim), The framerate is very good, but it's not 1000fps (I tried activating fps display but it doesn't showed anything).
I remove Profiler, because it generate a file and I don't want that, but I left debug on (and so there is the debug menu).

About the memcpy, I had a few also in mxaload.c, function InterpFrames. Is that a loading function too? I haven't really checked, just fixed the "bus error" when it happened (end of loading of level 1, after a first sound can be hear but still black screen).

@chino
Copy link
Member

chino commented Sep 5, 2013

Build Numbers

The build number used to be based off the svn revision but since we recently moved to github we're not yet sure what we're going to do about that.

As of right now I see 1.18.2546 as the latest on the download site .. but .. that should be bumped for recent changes we've made in the repo and also for whatever changes we're making for this branch.

For special builds though I usually just set PXBV in version.sh to a special name like pandora and then also in the download filename.

FPS

Hm so you enabled fps in the game and it doesn't show up? We should try to fix that I guess..

But I completely bet that your getting 100's of fps because Forsaken it self was developed in 95 and we have done updates and enabled all the advanced visuals but it's still ridiculously fast even with immediate mode :]

It's up to you if you want to build with optimizations or not but I would try playing with them turned off as a test cause I bet it's super fast and will deter any bugs that could happen.

Profiler

Yea I guess we can left PROFILE=0 by default which doesn't hurt but there was some other changes to the Makefile that we need to look at.

The gmon.out file it creates (right?) is used to pass to gprof to generate profiling data.

InterpFrames

InterpFrames seems to be called from ModelDisp short for display which is part of the main render loop yes. It's defined in mxaload.c which mx is the name used for mesh data files in Forsaken and a stands for animated since it provides a Gif like feature on surfaces.

I think almost anything that uses (maybe only float?) pointer arithmetic to iterate over structures may require your memcpy trick? I've honestly never dealt with such issues before so I'm not entirely sure about this is required or how the memcpy fixes it as I haven't had time to sit down and play/read about this some more. It does however sound rather odd ? I imagine there must be a compiler flag to disable this particular optimization that requires it or another option that makes it safer with less assumptions ? Still need to read into it a bit.. I wish I could emulate an arm system for testing ?

Question

What platform do you use to develop for Pandora? Linux / Windows / Mac ?

Can you help boot strap me to get egl or whatever else working locally?

@ptitSeb
Copy link
Author

ptitSeb commented Sep 8, 2013

I've started the git work, but haven't gone far yet. I'll probably need at least another week (because of RL, I'll not be at home).

About Devolopping on the Pandora, I have installed on Linux VM MESA v9 sources and compiled / installed the GLES + EGL part (disabling GL/GLX to not mess with actual drivers). That way I'm able to test most of the GLES code on a PC.
For ARM, I don't have setup anything, I just test on the Pandora. Some QEMU User mode can probably be used, but I never tried to.

@chino
Copy link
Member

chino commented Sep 16, 2013

RL = Remote Leave ?

That's fine I have been super busy my self.

I haven't checked this thread since my last post 12 days ago..

@chino
Copy link
Member

chino commented Oct 11, 2013

Hey how have things been going?

The memory alignment issues are also starting to affect me in my experiments with Emscripten which would allow us to compile the same code base directly into webgl edition! I think all these memcpy changes you made could be applied to work in Emscripten as well!

@ptitSeb
Copy link
Author

ptitSeb commented Oct 11, 2013

I've a bit busy with another project I wanted to complete.
I'll be back on Forsaken very soon now.

Emscipten, I heard about that tech, an amazing piece of software. Having
Forsaken on webgl is pretty cool indeed!

2013/10/11 Daniel Aquino [email protected]

Hey how have things been going?

The memory alignment issues are also starting to affect me in my
experiments with Emscripten which would allow us to compile the same code
base directly into webgl edition! I think all these memcpy changes you made
could be applied to work in Emscripten as well!


Reply to this email directly or view it on GitHubhttps://github.com//pull/280#issuecomment-26156343
.

@ptitSeb ptitSeb closed this Oct 12, 2013
@chino
Copy link
Member

chino commented Oct 12, 2013

Ah that's fine no rush.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port forsaken to run on ARM / Pandora open source gaming hand held.
2 participants