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

Generation of UID preventing loaded characters to display data in GUI #95

Open
ghost opened this issue Aug 19, 2017 · 2 comments
Open

Comments

@ghost
Copy link

ghost commented Aug 19, 2017

So let me start of saying this is my first time messing with php code, but after getting everything set up and done some reading i think i have found a pretty big problem in the code which at least maybe has lead to my problems.

From what i can read the UID creation for each "atom" are not being created to be unique for that type of object but it's instance.

/src/php/EPAtom.php

  function __clone()
    {
        // Ensure a clone object have a different atomUid from original 
        $this->atomUid = uniqid('Atom_'.$this->sanitize($this->name).'_');
    }

While most of the usage of the UID i have seen in the code use it to try to identify the same type of object, this works as long as it happens during the same runtime.

@EmperorArthur
Copy link
Collaborator

Hey, thanks for taking the time to look at the code. I'll try to explain what's going on and why.

Yes, that's deliberate, but it's (probably) not doing what you think it's doing.

PHP uses pass by reference

Here's a StackOverflow question talking about how PHP handles objects. The short answer is PHP does pass by reference, so clone has to be called explicitly. From my understanding, that code path is never actually used.

In principal, the objects being referenced by EPCharacter and everything under it are almost all* just references to the master lists. When a user is editing something, they aren't actually editing the character data (EPCharacter), they're editing the actual object.

That's why session creation takes so long. The first thing session creation does is populates all the objects from the database, so the user can see and modify them. After that the database is never used again.

Saving/Loading and master lists

The problem is how the save/load code works. Save actually stores the entire object in JSON, then load restores it. For example, this is how EPEgo handles loading skills:

**/src/php/EPEgo.php
$this->skills = array();
foreach($savePack['skillsSavePacks'] as $m){
$savedSkill = new EPSkill('','','','','');
$savedSkill->loadSavePack($m);
array_push($this->skills, $savedSkill);
}

Now, these will have their old Uid, so if they were traits they wouldn't actually map with the master list (which is what's being displayed to the users).

For example, here's what happens when a user changes the number of points they've put into a skill.

  • an ajax call is made to /src/Creator/version4/scripts/dispatcher.php:415:
    if(isset($_POST['changeSkill'])){
  • This then calls setSkillValue($id,$value) in /src/php/EPCharacterCreator.php
  • Which in turn looks up the skill by getSkillByAtomUid($id) still in /src/php/EPCharacterCreator.php

So, why does it work? Well that's because the skills in EPEgo are the master list.

Now I've mentioned traits, and how they are in a master list. So, how do those end up working? Well, every trait is displayed using getDynamicTraitLi in /src/Creator/Version4/other/traitLayer.php. That determines what traits the user has by calling isInArray from /src/php/EPAtom.php on every trait. Now isInArray uses $this->match from the same file to determine if an atom is in an array or not. That function does check if UIDs are the same. However, EPTrait (/src/php/EPTrait.php) overrides match so it uses other factors, and ignores UID!

The Future

Object identification is a mess right now. Fixing this problem is the whole point of issue #94. The ideal situation would be for every item to have a database_id. Then using that ID everywhere. At that point 90% of the loading code would be creating objects from the database, and only modify what the user has actually changed.

How did it get so complicated?

Well, originally most of the code passed the objects "name" from EPAtom everywhere. Which, to be fair, works for 90% of the use cases. The big problem was skills (and maybe gear). You couldn't have two skills of the same name, but with differing prefixes. For example "Electronics" and "Profession: Electronics" didn't work.

The other problem was that names have spaces in them. That causes a problem since html some HTML objects (like 'id') don't handle spaces well. It worked, but that's only because browsers will accept whatever was thrown at them.

Some UID code existed before hand, like the the clone function, but they weren't used very much. So, I cleaned the code up and started using them. Prior to this cleanup the name matching was mostly done directly in EPCharacterCreator.php and other places. I've tried to go with a more object oriented approach, but could have missed a comparison. If you see one, please let me know.

Hope that helped.

* Custom skills are an exception in that they're only stored per character.

@ghost
Copy link
Author

ghost commented Aug 20, 2017

edit: whoops, actually just re-read your comment and you essentially said what i describe here, i blame my ignorance on me staying up to long and reading your comment after just waking up

ah, im gonna try to give a better explanation of what i think i found, i was gonna do it with the other post but it was in the middle of the night, anyway like i said first time php so bear with me.

When i used the up to date version on https://www.cd-net.net/ep-character-creator/src/Creator/version4/index.php i encountered an error when loading an character from json that not all details where being displayed in the GUI.

So to investigate this i setup a local build and started to step trough the code process of loading a json structure and after confirming that all the data was loaded and stored correctly i found this

In morph.php (line 8) the code calls getmorphs() from EPCharacterCreator.php (line 1251) which then returns morphs (line 41), this array gets generated at session start and all the UID's will be different from session to session as the comment says.

This is because when the array containing all the standard morphs gets generated by getListMorphs() in EPListProvider.php (line 610) that function calls the constructor for EPMorph (line 125) which itself calls its parents constructor EPAtom (line 50) that calls the php function uniqid() and assigns it the UID for that Atom which in this case is a Morph.

This is a problem because then at morph.php (line 47) and at (line 49) a comparison is being made with the characters morph using isInArray() from Atom.php (line 142), isInArray() compares the UID of the two objects, which as i explained is going to wary from session to session so if the character has been loaded from json and it was made during an other session the comparison is never gonna return true and therefore the character from a different session is not gonna be displayed correctly

Since the UID generation happens in the Atom constructor this is gonna happen to any object using UID.

So there you have it, what i maybe think i have found.

@ghost ghost changed the title disparity in UID creation and final usage Generation of UID preventing loaded characters to display data in GUI Aug 20, 2017
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

No branches or pull requests

1 participant