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.

Sign up for GitHub

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

Jump to bottom

respect textures-header-only for embedded textures #1448

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

respect textures-header-only for embedded textures #1448

wants to merge 1 commit into from

Conversation

lachbr
Copy link
Contributor

@lachbr lachbr commented Jan 22, 2023

Issue description

The textures-header-only config option is not respected for textures that have been embedded in a bam file. This is likely an oversight, as the common use case for textures-header-only is to reduce memory usage for processes that don't need the texture data, like servers.

Solution description

Simply skip over the bytes containing the image data if textures-header-only is set.

Checklist

I have done my best to ensure that…

  • …I have familiarized myself with the CONTRIBUTING.md file
  • …this change follows the coding style and design patterns of the codebase
  • …I own the intellectual property rights to this code
  • …the intent of this change is clearly explained
  • …existing uses of the Panda3D API are not broken
  • …the changed code is adequately covered by the test suite, where possible.

@codecov
Copy link

codecov bot commented Jan 22, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 16.15%. Comparing base (a88b6ee) to head (a196aec).
Report is 498 commits behind head on master.

Files with missing lines Patch % Lines
panda/src/gobj/texture.cxx 0.00% 7 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1448      +/-   ##
==========================================
- Coverage   16.15%   16.15%   -0.01%     
==========================================
  Files        3745     3745              
  Lines      364122   364098      -24     
==========================================
- Hits        58836    58805      -31     
- Misses     305286   305293       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rdb
Copy link
Member

rdb commented Feb 7, 2023

Conflicted about this, because it is lossy. Normally you don't lose information because you retain the filename to load from. With this change you can no longer safely roundtrip bam files without losing data.

Can you elaborate further on your use case?

@Maxwell175
Copy link
Contributor

My use case for this PR was saving memory on the headless/server side of things, while still being able to reuse the same bam files as on the client.

@rdb
Copy link
Member

rdb commented Feb 7, 2023

That sounds useful in principle, but does textures-header-only actually currently accomplish that? It creates a RAM image filled with a solid blue colour, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lachbr @rdb @Maxwell175