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

Remove PROPERTY_HINT_IMAGE_COMPRESS constants #67688

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Oct 20, 2022

Removes PROPERTY_HINT_IMAGE_COMPRESS_LOSSY and PROPERTY_HINT_IMAGE_COMPRESS_LOSSLESS. These do not actually do anything, at all. These were used in 3.x, but are no longer necessary now.

@Mickeon Mickeon requested review from a team as code owners October 20, 2022 20:32
@Calinou Calinou added this to the 4.0 milestone Oct 20, 2022
@nikitalita
Copy link
Contributor

What about lossy-compressed webps? Textures can still be encoded as those.

@nikitalita

This comment was marked as outdated.

@Mickeon
Copy link
Contributor Author

Mickeon commented Oct 20, 2022

What about lossy-compressed WEBPs? Textures can still be encoded as those.

Don't worry, these are not import options. I reiterate, these property hints do not do anything in the engine.

@nikitalita

This comment was marked as outdated.

@Mickeon
Copy link
Contributor Author

Mickeon commented Oct 20, 2022

This PR's failing check has made me discover that PROPERTY_HINT_INT_IS_POINTER and PROPERTY_HINT_ARRAY_TYPE are exposed in the wrong order and should be swapped. Putting it out here as a mental note.

@Mickeon Mickeon force-pushed the i-forgror-☠️☠️ branch from ef5ad2c to bb2b13f Compare October 20, 2022 22:01
@nikitalita
Copy link
Contributor

I am sorry, I had thought that this also removed COMPRESS_LOSSLESS and COMPRESS_LOSSY; those values would remain unchanged and those are still encoded in the texture. The PropertyHint is only set after the resource is imported. I withdraw my objection.

@nikitalita
Copy link
Contributor

After some searching through the code, I don't see any Object that encodes PropertyHint directly into a resource. I don't think this will break any Godot resources; it might break resources in extensions that do that, though. It might also break third-party editor tools that rely on the PropertyHint enum they get from the language server directly.

In either case, the enum values started changing back in April, here: #60458

At the time, It was decided to just merge and cleanup later. So I guess that's something to worry about later? 🤷

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Approved in PR review meeting, those are unused (also unused in 3.x but we don't want to break compat there).

These were used in 3.x but there's no reference of them in the codebase, at all.
@Mickeon Mickeon force-pushed the i-forgror-☠️☠️ branch from bb2b13f to b4324e7 Compare November 15, 2022 14:46
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 15, 2022

Rebased.

@akien-mga akien-mga merged commit 09efe15 into godotengine:master Nov 15, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants