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

Fix unwanted file changes #1770

Closed
wants to merge 13 commits into from
Closed

Fix unwanted file changes #1770

wants to merge 13 commits into from

Conversation

BertovDev
Copy link
Contributor

@BertovDev BertovDev commented May 15, 2024

Motivation

  • Text mesh pro problems.

TextMeshPro dynamic font assets have a very annoying habit of saving their dynamically generated binary data in the same text file as their configuration data. This causes massive headaches for version control.

  • Danger zone problem

Renderer.sharedMaterial returns the actual instance(s) of the Material at renderer.sharedMaterials[0] that is on the object.
This can be modified, and it will affect every use of this Material in the Scene.

renderer.material, on the other hand, creates a copy of renderer.sharedMaterials[0]

So with this we are not overriding the material saved in the scene.

Summary of changes

  • Prevent dynamics fonts changes to be saved
  • Use material instead of shaderMaterial to do changes.
  • Set rotation after instantiating the model Denial_AOE.
  • Regenerate each font again as Static.

How has this been tested?

Normal play the game, no changes of fonts and the danger zone should appear in the source control.

Checklist

  • I have tested the changes locally.
  • I have tested the whole game after applying the changes, not only the affected areas.
  • I self-reviewed the changes on GitHub, line by line.
  • I have tested the changes in another devices.
    • Tested in iOS.
    • Tested in Android.
  • This change requires new documentation.
    • Documentation has been added/updated.

Copy link
Contributor

@alfopisano alfopisano left a comment

Choose a reason for hiding this comment

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

Restores any changes from the terminal. Played on unity desktop, after the battle ended I stopped the game and saved any possible changes. When heading back to the terminal this files were modified:
Screenshot 2024-05-15 at 16 27 09

Copy link
Contributor

@alfopisano alfopisano left a comment

Choose a reason for hiding this comment

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

Tested as described previously and encountered the following problems:

  • This file was modified:
Screenshot 2024-05-15 at 18 23 38
  • I asume it has to do with the change above but some font were looking weird:
Screenshot 2024-05-15 at 18 22 00 Screenshot 2024-05-15 at 18 21 34

@BertovDev
Copy link
Contributor Author

This file was modified:

Did you restart unity?

@alfopisano
Copy link
Contributor

This file was modified:

Did you restart unity?

I did not, I would try that

@alfopisano
Copy link
Contributor

Tried restarting unity and the problem persisted.

Copy link
Contributor

@AngieDutra AngieDutra left a comment

Choose a reason for hiding this comment

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

In addition, I'm having the same issue. Pink means missing material, maybe you forgot to save something in the fix process?
Screenshot 2024-05-20 at 11 48 44

@AngieDutra
Copy link
Contributor

Or maybe you need to replace the fonts one by one through the game?

@AngieDutra AngieDutra marked this pull request as draft May 20, 2024 14:52
@tkz00
Copy link
Contributor

tkz00 commented Jun 4, 2024

This PR is trying to solve two different issues, the fonts one is being handled here: Fix dynamic font assets changing version control, the materials/textures one will be investigated and tackled later on.

@tkz00 tkz00 closed this Jun 4, 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.

None yet

4 participants