Toggling entity visibility is undoable and fix saving visible false

[vincentfretin]

This fixes one part of #793

[kfarr]

@vincentfretin ok i made an attempt to save/load visible. It worked in my testing but I would appreciate you to look it over again. I was worried about javascript's confusion sometimes to detect the existence of a property whose value is false vs. a property that does not exist, but entity.getAttribute('visible') === false seems to result in the desired behavior.

[vincentfretin]

The visible key should really be in the components object, so not a correct fix, but you gave me an idea of the real issue.

[vincentfretin]

There were two issues here to be able to export "visible": "false"
First is the condition if (modifiedProperty) that should check explicitly if (modifiedProperty !== null)
After fixing this, it wrongly exported "visible": ""
The second issue was the isEmpty(modifiedProperty). This function didn't check for an object before using Object.keys. Object.keys(false) gave [] so [].length === 0 and was putting "" in the export.
Also in javascript, to detect if a variable is an object, typeof variable === "object" is not enough, because null and an array are also this type, so you need to check for not null and not array before using Object.keys here.

[kfarr]

thanks, this makes sense in theory but I can't follow the details enough to validate the logic.

So instead thinking out loud, one way to quickly test this is to compare scene json export from before / after the fix on a complicated scene (without hidden visibility, since that is new behavior) and see if the output is identical?

[kfarr]

here are results so far from my testing

old and new comparing json output for the same scene

strange thing:

• in one case, "scale" component values now appear in the new version where they didn't before with mixin = "stencils bike-arrow" (pic2)
• in another case, "scale" component values do not appear in the new version where they did before for "stencils word-taxi" (pic4)
• in any case, the scene looks the same when loaded, as the scale values are superfluous since they apply to an entity whose scale will again be set by the specified mixins

minor things:

• position and rotation switch places, and a few other props change ordering, no big deal (pic1)
• previously some components included a "visible": "" json entry, this persists but the order changes, no big deal just fyi (pic3)

pic1

pic2

pic3

pic4

[vincentfretin]

Yes, all the things you just reported existed before this change.
The scale component appearing sometimes, I reproduced it before the change. That's another issue I didn't want to adresse in this PR. I have my guess where the issue may be in the code, probably in

3dstreet/src/json-utils_1.1.js

Lines 312 to 322 in afb7aa2

	const mixinSkipProps = [
	'src',
	'atlas-uvs',
	'gltf-model',
	'gltf-part',
	'shadow'
	];
	if (mixinsData && mixinSkipProps.includes(mixinCompName)) {
	// skip properties, if they exists in element's mixin
	return null;
	}

where we have an hard coded list of mixin properties to ignore instead of checking dynamically if componentName is used in one of the mixin and the value didn't change from the value of the mixin.

[vincentfretin]

You can create a separate issue if you want to fix removing the scale, geometry, material components if that's the same value as the mixin, make the component order deterministic, or round the coordinates of position and rotation.
As far as I can tell this PR is okay to be merged.

[vincentfretin]

exporting "visible": "" when you toggled twice an entity is expected, the entity now have the visible component and will be exported, it's empty string because the value true is the default for this component, so this the expected export here.