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

Modernize old templates #746

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

nezuo
Copy link
Contributor

@nezuo nezuo commented Jul 24, 2023

This PR is meant to start a discussion on improvements we can make to the old templates. If anyone disagrees with a change I made, let me know!

Here are the changes I made:

  • .gitignore updates:
    • Lock files in all directories are ignored now, instead of only the root directory.
    • The binary format is now ignored for place/model files.
    • The place/model files don't need a specific name to be ignored anymore.
  • Added newlines to all the files (except the project files, since rojo fmt-project doesn't add a newline).
  • The READMEs now recommend the binary format instead of XML.
  • I ran rojo fmt-project on the place project file. I'm not sold on this, the output is a little strange and rojo fmt-project isn't even deterministic.
  • I removed the FilteringEnabled property from the place project file since it's always on by default now.
  • I renamed the model template to library to better reflect what it does.

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

I think it's actually probably fine to leave the template as model instead of library. It's the goto alternative to place, so it makes sense to me.

@boatbomber
Copy link
Member

Should we update the place to match the new Baseplate of Studio?

@nezuo
Copy link
Contributor Author

nezuo commented Aug 15, 2023

It's a matter of preference but the current template we have has outdated Lighting properties. I think we should update it. Does anyone have any reservations about updating it?

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