Misc. Questions

If you wish to contribute to the Ace Attorney Online Game Creation Engine open source project, or just know more about the way it is developed, this is the place.

Moderator: EN - Forum Moderators

User avatar
Enthalpy
Community Manager
Posts: 5186
Joined: Wed Jan 04, 2012 4:40 am
Gender: Male
Spoken languages: English, limited Spanish

Misc. Questions

Post by Enthalpy »

Current Unas-Specific Questions Lists:
None!
_____________________________________________________

This is a topic for miscellaneous questions that aren't going to elicit a discussion long enough to get their own topic, likely won't be worth much after the question is answered for you, but don't quite fit in an issue tracker discussion, or effectively need an answer from Unas, and this is the best way to get a hold of him.

Starting with the question that prompted me to write this...

All .php files load the bridge.js.php script. Because it's loading as a JS script and not through PHP's include or require, bridge.js.php doesn't inherit the calling page's variables, such as $user. When bridge.js.php loads common.php, it tries to set the user's current language. However, because this was marked as "PHPBB NOT NEEDED," this page doesn't know anything about the user, causing an error.

There are many possible fixes, but I'm not sure what the best one is. My intuition is that bridge.js.php shouldn't know anything about a user. PHPBB is simply not needed. In that case, the only common.php functions that make sense for it to have are the first three, and the only one it uses are the first one.

My instinct is to simply either:
* Replace the include common call with an include config call; This is the least information that bridge.js.php needs to know to keep functioning.
* Move everything below "include config" into a new file, and have that file be included unless PHPBB_NOT_NEEDED is defined.

I'm leaning toward the second option, but I would like some other opinions before I commit this.
[D]isordered speech is not so much injury to the lips that give it forth, as to the disproportion and incoherence of things in themselves, so negligently expressed. ~ Ben Jonson
User avatar
Unas
Admin / Site programmer
Posts: 8854
Joined: Tue Jul 10, 2007 4:43 pm
Gender: Male
Spoken languages: Français, English, Español
Contact:

Re: Misc. Questions

Post by Unas »

To answer the question, bridge.js.php is a purely technical script, that feeds the initial setup (file versions, name of the root script to launch, etc.) into the JS engine. It does not need to know about any functional thing like the current user.
That's why, if it has to load common.php itself (which is the case if it's called from a script tag URL like in the layout_header), it sets PHPBB_NOT_NEEDED so as to avoid some useless and costly processing.

In your case, several level of fixes are possible.

1. UserDataHandler:getCurrentUser should probably just return null if $user is unset : language_backend expects that and would manage it gracefully.

2. Anyway, bridge.js.php should not require the language stuff from common.php either...
I think common.php should be split actually, between "common.php" with the technical tools that have no dependency and practically every piece of PHP code here needs (includeScript, __autoload, escapeJSON, include of config.php), and a new "common_render.php", including common and adding the stuff which is required only by scripts that render something to the client (user data, language, etc)
bridge would depend on common.php, whereas pages would depend on common_render, something like that.
ImageImageImage
If knowledge can create problems, it is not through ignorance that we can solve them.
Si le savoir peut créer des problèmes, ce n'est pas l'ignorance qui les résoudra. ( Isaac Asimov )
AceAttorneyMaster111
Posts: 468
Joined: Sat Sep 27, 2014 6:46 pm
Gender: Male
Spoken languages: English, français, un poco de español, עברית
Location: USA

Re: Misc. Questions

Post by AceAttorneyMaster111 »

Is there a reason you haven't accepted or declined my pull request #13 and Enthalpy's pull request #5? Just wondering... :hobo:
User avatar
Unas
Admin / Site programmer
Posts: 8854
Joined: Tue Jul 10, 2007 4:43 pm
Gender: Male
Spoken languages: Français, English, Español
Contact:

Re: Misc. Questions

Post by Unas »

Well, forgive me if I'm a little blunt, but the main issue with your PRs is that it looks like you never test any of your changes, and I don't really have time to fetch them and test them myself.

You see, the correct way to go with things is :
  • A contributor makes a change on a new branch of his fork.
  • That contributor tests his change to make sure it's working.
  • That contributor opens a pull request. (update and re-test if necessary)
  • The pull request is merged onto develop.
  • When making a new release from develop, I test the new version to make sure the changes still work and did not break anything else.
In Enthalpy's case, I know I can usually trust him about testing changes : he has a local git clone, he uses static testing, and just made a fix on mocked testing precisely to have it work on his machine (which apparently has stricter settings than mine).
If I'm confident something has been reasonably tested, I can merge it.

In your case, the changes are usually not tested. Enth and I are repeatedly asked you "did you test that change ?" and you never ever answered it. Plus, it's pretty clear that you are still not using git, always editing using the web interface (against my recommendations) - which supports the idea that you are not testing anything on your machine.
That's why I can't trust your code : I'm pretty sure it wasn't tested, and I would have to fetch your fork and test it myself before merging. That's a lot more time than just clicking the merge button. :|


You have to understand that my recommendations on how to contribute are here for a reason. Using git for development on AAO is not optional, neither is testing your changes locally before pushing.
That's the whole reason I created two new testing environments (static testing and mocked testing) before going public : I know that a fully integrated environment is hard to setup for a newbie developer, so I've given you simpler ways to test changes.
I made an effort to make testing simpler for other contributors (as far as I'm concerned, I've had a fully integrated install set up on all of my computers for ages, so I really didn't need that), now it's your turn to make an effort and actually test your stuff.

You can't contribute by just throwing in code lines here and there and expect the others to fix it : to make useful contributions, you first need to make sure that they work. :wink:
ImageImageImage
If knowledge can create problems, it is not through ignorance that we can solve them.
Si le savoir peut créer des problèmes, ce n'est pas l'ignorance qui les résoudra. ( Isaac Asimov )
User avatar
kwando1313
Posts: 7684
Joined: Tue Jul 22, 2008 6:33 pm
Gender: Male
Spoken languages: English, Français (un peu), Ancient Belkan
Location: Uminari City

Re: Misc. Questions

Post by kwando1313 »

Speaking of Enthalpy's change to the mocked_install... Will that be merged in soon? (Mostly because I actually want to start doing some coding for AAO again. xP)
Avatar made by Rimuu~

Image

"The Knight of the Iron Hammer, Vita, and the Steel Count, Graf Eisen. There's nothing in this world we can't destroy."
AceAttorneyMaster111
Posts: 468
Joined: Sat Sep 27, 2014 6:46 pm
Gender: Male
Spoken languages: English, français, un poco de español, עברית
Location: USA

Re: Misc. Questions

Post by AceAttorneyMaster111 »

Unas wrote:Well, forgive me if I'm a little blunt, but the main issue with your PRs is that it looks like you never test any of your changes, and I don't really have time to fetch them and test them myself.

You see, the correct way to go with things is :
  • A contributor makes a change on a new branch of his fork.
  • That contributor tests his change to make sure it's working.
  • That contributor opens a pull request. (update and re-test if necessary)
  • The pull request is merged onto develop.
  • When making a new release from develop, I test the new version to make sure the changes still work and did not break anything else.
In Enthalpy's case, I know I can usually trust him about testing changes : he has a local git clone, he uses static testing, and just made a fix on mocked testing precisely to have it work on his machine (which apparently has stricter settings than mine).
If I'm confident something has been reasonably tested, I can merge it.

In your case, the changes are usually not tested. Enth and I are repeatedly asked you "did you test that change ?" and you never ever answered it. Plus, it's pretty clear that you are still not using git, always editing using the web interface (against my recommendations) - which supports the idea that you are not testing anything on your machine.
That's why I can't trust your code : I'm pretty sure it wasn't tested, and I would have to fetch your fork and test it myself before merging. That's a lot more time than just clicking the merge button. :|


You have to understand that my recommendations on how to contribute are here for a reason. Using git for development on AAO is not optional, neither is testing your changes locally before pushing.
That's the whole reason I created two new testing environments (static testing and mocked testing) before going public : I know that a fully integrated environment is hard to setup for a newbie developer, so I've given you simpler ways to test changes.
I made an effort to make testing simpler for other contributors (as far as I'm concerned, I've had a fully integrated install set up on all of my computers for ages, so I really didn't need that), now it's your turn to make an effort and actually test your stuff.

You can't contribute by just throwing in code lines here and there and expect the others to fix it : to make useful contributions, you first need to make sure that they work. :wink:
I've tried downloading the latest stable version of Git 300 gazillion times. I've also tried downloading the mocked install 300 gazillion times, even before Issue 123. It never works on my computer.
User avatar
Unas
Admin / Site programmer
Posts: 8854
Joined: Tue Jul 10, 2007 4:43 pm
Gender: Male
Spoken languages: Français, English, Español
Contact:

Re: Misc. Questions

Post by Unas »

Then ask for help in this forum ! Explain what your issues are, and let's fix them ! ;-)

I'm fine with taking a little time at first to get you started properly. We could update the dev wiki with all the steps you had to do, all the problems you had, so that someone else benefits from this experience.

But I'm not fine with spending time repeatedly because none of your development is tested.
ImageImageImage
If knowledge can create problems, it is not through ignorance that we can solve them.
Si le savoir peut créer des problèmes, ce n'est pas l'ignorance qui les résoudra. ( Isaac Asimov )
User avatar
Enthalpy
Community Manager
Posts: 5186
Joined: Wed Jan 04, 2012 4:40 am
Gender: Male
Spoken languages: English, limited Spanish

Re: Misc. Questions

Post by Enthalpy »

kwando1313 wrote:Speaking of Enthalpy's change to the mocked_install... Will that be merged in soon? (Mostly because I actually want to start doing some coding for AAO again. xP)
I'd need to re-pull it. I'd love to do some more advanced testing on it, but this is a blocker-class problem, and I don't expect to have internet access after a few hours! I'll do a pull request with everything that I know is killing mocked_install, and do a more thorough testing of mocked_install later.

This would also include cataloging exactly what you need the test_install for. Creating a sequence/series, for example, isn't the kind of thing that you think would require the full version, but it is!
AceAttorneyMaster111 wrote:I've tried downloading the latest stable version of Git 300 gazillion times. I've also tried downloading the mocked install 300 gazillion times, even before Issue 123. It never works on my computer.
Issue 123 has existed since open-source AAO launched. It was initially reported here. It just took some time to move it on the Issue Tracker. If the issue you're having isn't that one, please explain what it is.

EDIT: A partial fix to 123 is up. The editor and player are still down, but I'll work on that. If you want to get those fixed, you need to get the TrialFileHandler to take a blank trial differently, though I'll work on that to. What's up now will be enough for AAM to test his commits, if accepted. I don't expect to have internet access beyond e-mail checking between now and Sunday morning.
[D]isordered speech is not so much injury to the lips that give it forth, as to the disproportion and incoherence of things in themselves, so negligently expressed. ~ Ben Jonson
User avatar
Enthalpy
Community Manager
Posts: 5186
Joined: Wed Jan 04, 2012 4:40 am
Gender: Male
Spoken languages: English, limited Spanish

Re: Misc. Questions

Post by Enthalpy »

I'm working on the fix to the blank trials problem I mentioned in my last commit (which is also turning into a restructuring of the converters), and I'd like to do some testing. Could I get a copy of a non-converted Def1, Def4, and Def5 trial to test on? 626, 8997, and 14549 should do, but I'm not picky.
[D]isordered speech is not so much injury to the lips that give it forth, as to the disproportion and incoherence of things in themselves, so negligently expressed. ~ Ben Jonson
User avatar
Enthalpy
Community Manager
Posts: 5186
Joined: Wed Jan 04, 2012 4:40 am
Gender: Male
Spoken languages: English, limited Spanish

Re: Misc. Questions

Post by Enthalpy »

I'm about done with the fix (after I get the test trials), but stumbled upon something a bit odd: All the conveters have getProfile, getEvidence, getFrame, and getPlace utility functions that don't seem to be used, even in older versions of the code. Is there some reason why these are around, or am I free to get rid of them?
[D]isordered speech is not so much injury to the lips that give it forth, as to the disproportion and incoherence of things in themselves, so negligently expressed. ~ Ben Jonson
User avatar
Unas
Admin / Site programmer
Posts: 8854
Joined: Tue Jul 10, 2007 4:43 pm
Gender: Male
Spoken languages: Français, English, Español
Contact:

Re: Misc. Questions

Post by Unas »

There was a reason at first, but after a while I thought it through and forgot the idea - or rather this way of doing it.
You should be able to get rid of these methods.

That being said, to be honest, these converters are a mess, and as long as they are working I'd be very careful when touching them :-P
ImageImageImage
If knowledge can create problems, it is not through ignorance that we can solve them.
Si le savoir peut créer des problèmes, ce n'est pas l'ignorance qui les résoudra. ( Isaac Asimov )
User avatar
Enthalpy
Community Manager
Posts: 5186
Joined: Wed Jan 04, 2012 4:40 am
Gender: Male
Spoken languages: English, limited Spanish

Re: Misc. Questions

Post by Enthalpy »

I am being very careful, and making them less of a mess is part of the idea! I've cut down about 600-700 lines of code by creating an abstract FormattingBase class and combining FormattingDef4 and FormattingDef5 into a single FormattingSerialized class that knows when to follow the Def4 behavior and when to follow the Def5 behavior.

I'll do one last comparison between the current version and my version before making the commit, as well as running some actual tests, once I get the test files to work with.
[D]isordered speech is not so much injury to the lips that give it forth, as to the disproportion and incoherence of things in themselves, so negligently expressed. ~ Ben Jonson
User avatar
Unas
Admin / Site programmer
Posts: 8854
Joined: Tue Jul 10, 2007 4:43 pm
Gender: Male
Spoken languages: Français, English, Español
Contact:

Re: Misc. Questions

Post by Unas »

Ah, yes, sorry, I forgot to answer about the files.

In fact, you can get them yourself, since you are an admin over the English section.
If you load any trial over which you are an admin (here, any English trial) in either the player or editor, the trial_information variable available in your web browser contains one more field : trial_information.file_path

You can then fetch the file by appending this file path to the AAO domain (eg. http://aceattorney.sparklin.org/the/file_path )
ImageImageImage
If knowledge can create problems, it is not through ignorance that we can solve them.
Si le savoir peut créer des problèmes, ce n'est pas l'ignorance qui les résoudra. ( Isaac Asimov )
AceAttorneyMaster111
Posts: 468
Joined: Sat Sep 27, 2014 6:46 pm
Gender: Male
Spoken languages: English, français, un poco de español, עברית
Location: USA

Re: Misc. Questions

Post by AceAttorneyMaster111 »

I think I figured out why the mocked install isn't working. Where can I get a server that works with PHP and PHP SQlite? Or do I probably already have one?
User avatar
Enthalpy
Community Manager
Posts: 5186
Joined: Wed Jan 04, 2012 4:40 am
Gender: Male
Spoken languages: English, limited Spanish

Re: Misc. Questions

Post by Enthalpy »

Second option after googling "PHP Server."

The first two examples are the most important parts.
[D]isordered speech is not so much injury to the lips that give it forth, as to the disproportion and incoherence of things in themselves, so negligently expressed. ~ Ben Jonson
Post Reply