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

Retain loaded position of an imported obj file #283

Closed
wants to merge 3 commits into from

Conversation

mvriel
Copy link
Contributor

@mvriel mvriel commented Oct 7, 2024

In this PR, I have made the following changes:

  • a bit of cleanup
  • The HierarchicalLayerGameObject will now interact with the WorldTransform and use coordinates instead of transform.position; this will reduce edge cases involving shifts
  • The SpawnPosition is now determined in the Awake instead of it being after importing the obj to make sure that positions set by external forces - such as loading a project- won't be overwritten by this default

…o issues arise from shifting. The transform position was used, but that can be awkward as soon as a shift occurs.

Unfortunately that didn't fix the situation with the erroneous positioning.
…sition set through the loading of a project, or another external method
@mvriel mvriel marked this pull request as ready for review October 8, 2024 08:54
@mvriel mvriel changed the title Fix/3d object save and load Retain loaded position of an imported obj file Oct 8, 2024
@mvriel mvriel requested a review from TomSimons October 8, 2024 08:57
Copy link
Contributor

@TomSimons TomSimons left a comment

Choose a reason for hiding this comment

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

Ik heb een paar comments met mogelijke problemen waarvan ik denk dat het beter is om deze even te dubbelchecken voordat ik het approve

if (newAngles == transform.eulerAngles) return;

worldTransform.Rotation = Quaternion.Euler(newAngles);
transform.rotation = worldTransform.Rotation;
Copy link
Contributor

Choose a reason for hiding this comment

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

gaat dit goed met de bolling van de aarde? moet er niet ook een ToUnity() functie worden aangeroepen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

worldTransform.Rotation is een Quaternion die -als ik @mvangog goed begrepen heb- de rotatie van de gameobject is na toepassen van de bolling

Copy link
Contributor

Choose a reason for hiding this comment

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

Maar dan zou de gebolde rotatie nu toepast worden op het gameObject, waardoor een object op de evenaar dus 90º gedraaid zou zijn?

Copy link
Contributor Author

@mvriel mvriel Oct 8, 2024

Choose a reason for hiding this comment

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

🤔 Ik zou momenteel niet zo goed weten hoe dit te testen; we doen nu nog alles met RD waardoor de bolling niet wordt toegepast..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En anders draai ik de rotatie dingen even terug; voor de zekerheid

Copy link
Contributor

Choose a reason for hiding this comment

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

worldtransform.rotation is de rotatie van het gameobject in zijn eigen coordinatenstelsel. (dus vóór het toepassen van de "bolling", zoals mike het noemt)
de rotatie in unity wordt uitgerekend met worldtransform.coordinate.RotationToUnityUp() * worldtransform.rotation

var transformProperty = (TransformLayerPropertyData)properties.FirstOrDefault(p => p is TransformLayerPropertyData);
if (transformProperty != null)
var transformProperty = properties.OfType<TransformLayerPropertyData>().FirstOrDefault();
if (transformProperty == default(TransformLayerPropertyData)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

waarom niet expliciet transformProperty == null hier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Als je in properties.OfType<TransformLayerPropertyData>().FirstOrDefault() 'ofType' gebruikt dan heb je het voordeel dat de conversie naar TransformLayerPropertyData automatisch goed gaat, dus heb je geen casting meer nodig, maar het betekent wel dat FirstOrDefault niet perse een null value teruggeeft, maar de 'default' van TransformLayerPropertyData. Dus als je FirstOrDefault gebruikt, dan is het veiliger om default(...) te gebruiken zodat je zeker weet dat de check klopt (en default is regelmatig null, maar niet altijd; zoals bij structs IIRC)

Copy link
Contributor

Choose a reason for hiding this comment

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

ja, default is alleen null als de Type nullable is.

maar voor mij geeft een == null aan dat er niks is gevonden, en hoe ik dit lees is dat er een default waarde is gevonden, wat niet per se null hoeft te zijn.

verder is dit geen blokkerende issue, maar ik vroeg me gewoon af wat het voordeel hiervan was, al begrijp ik jouw uitleg nog niet helemaal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FirstOrDefault geeft geen null terug als die niks vind, die geeft -zoals de methode naam aangeeft- de default terug als die niks vind. Toevallig is dat voor nullable type 'null'; maar het is met FirstOrDefault vaak beter -zeker in combinatie met ofType omdat dan default daadwerkelijk over die type gaat- om te checken op default(type).

Het verschil zit hem in dit ofType als je die niet gebruikt, dan weet Linq niet welk type resultaat eruit gaat komen, dus dan is default 'null'. Maar als je ofType gebruikt, dan krijg je automatische typecasting, maar dus ook van je default; en dus moet je checken op default(type) en niet op null

}

private void Update()
{
// We cannot use transform.hasChanged, because this flag is not correctly set when adjusting this transform using runtimeTransformHandles, instead we have to compare the values directly
// Check for position change
if (transform.position != previousPosition)
if (worldTransform.Coordinate.ToUnity() != previousPosition.ToUnity())
Copy link
Contributor

Choose a reason for hiding this comment

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

kan er niet een directe coordinate vergelijking gedaan worden (of worden toegevoegd)? dit lijkt me een beetje vatbaar voor floating point errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

De Coordinate class heeft geen Equality operator implementatie; daar liep ik tegenaan toen ik dit stuk omschreef. Maar ik wilde de scope ook niet heel erg vergroten door operator overrides in de Coordinate converter package toe te voegen

set;
}

public Coordinate Coordinate { get; set; } = new(CoordinateSystem.RDNAP);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ik meen me te herinneren dat deze constructor kapot was. Kan je even met Martijn dubbelchecken of dat opgelost is? Ik heb hier een keer uren aan besteed omdat ik niet begreep wat er fout was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ik heb geen problemen ervaren bij testen, maar goed dat je dat dan aangeeft. @mvangog: weet jij dit toevallig nog?

Copy link
Contributor

@mvangog mvangog Oct 8, 2024

Choose a reason for hiding this comment

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

het coordinaat wrdt nu geinitiliseerd as rdnap (is 3d) met 0 waarden. het gevaar hierbij is dat, als deze om een of andere reden gebruikt gaat worden met transformaties, er rare dingen gaan gebeuren omdat er 3 waarden aanwezig horen te zijn. (dat is volgens mij het probleem waar Tom toen tegenaan liep, of het was rdnap met 2 waarden)
ik zou hier een new coordinate(0,0,coordinateSystem.Undefined) doen. de transformatie-tooling weet dat ie niets kan met coordaintesystem.undefined.

@mvriel mvriel marked this pull request as draft October 10, 2024 08:36
@mvriel
Copy link
Contributor Author

mvriel commented Oct 10, 2024

Naar draft geconvert omdat ik nog de rotatie changes ongedaan moet maken

@mvriel
Copy link
Contributor Author

mvriel commented Oct 21, 2024

Het fixen van de WorldTransform zaken is geen prio, dus ik ga dat deel afschieten en los oppakken als er meer vraag naar is. De kern van deze PR is dat de ObjSpawner iets anders werkt en dat heb ik afgesplitst naar #298

@mvriel mvriel closed this Oct 21, 2024
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.

3 participants