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

Implement ColorTable struct #246

Merged
merged 2 commits into from May 14, 2022
Merged

Implement ColorTable struct #246

merged 2 commits into from May 14, 2022

Conversation

Barugon
Copy link
Contributor

@Barugon Barugon commented Jan 26, 2022 •
edited

Implement a ColorTable struct and add a RasterBand::color_table method.

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md.

ChristianBeilschmidt
ChristianBeilschmidt requested changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@ChristianBeilschmidt ChristianBeilschmidt left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. I have some comments.

src/raster/rasterband.rs Show resolved Hide resolved
src/raster/rasterband.rs Outdated Show resolved Hide resolved
src/raster/rasterband.rs Outdated Show resolved Hide resolved
ChristianBeilschmidt
ChristianBeilschmidt requested changes Jan 30, 2022
View reviewed changes
src/raster/tests.rs Outdated Show resolved Hide resolved
src/raster/tests.rs Outdated Show resolved Hide resolved
ChristianBeilschmidt
ChristianBeilschmidt requested changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@ChristianBeilschmidt ChristianBeilschmidt left a comment

Choose a reason for hiding this comment

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

From my point of view, this looks now very good.
The really last comment from my side, then it is mergeable for me.

src/raster/rasterband.rs Outdated Show resolved Hide resolved
@ChristianBeilschmidt
Copy link
Contributor

1 workflow awaiting approval

@jdroenner could you approve the CI for this PR?

Barugon
Barugon commented Feb 4, 2022
View reviewed changes
src/raster/rasterband.rs Outdated Show resolved Hide resolved
@Barugon
Copy link
Contributor Author

Barugon commented Feb 24, 2022

Any chance of getting this merged?

@jdroenner
Copy link
Member

yes i'm just very busy at the moment. will try to look into it this week.

@Barugon
Squashed commit of the following:
fbd9aff 
commit 147809c
Merge: 6e63f0f b0ebd1d
Author: Barugon <barugon@dungeonbox.net>
Date:   Fri May 13 10:46:52 2022 -0700

    Merge branch 'master' into barugon/color_table

commit 6e63f0f
Merge: 4e75d8d d8aa19a
Author: Barugon <barugon@dungeonbox.net>
Date:   Fri May 13 00:43:18 2022 -0700

    Merge branch 'master' into barugon/color_table

commit 4e75d8d
Merge: 9362add 8cb6e92
Author: Barugon <barugon@dungeonbox.net>
Date:   Wed Apr 20 09:50:59 2022 -0700

    Merge branch 'master' into barugon/color_table

commit 9362add
Author: Barugon <barugon@dungeonbox.net>
Date:   Wed Feb 16 18:12:35 2022 -0800

    Export some necessary items

commit 1f7b73d
Merge: d2d0df4 6cb085a
Author: Barugon <barugon@dungeonbox.net>
Date:   Sat Feb 12 17:40:50 2022 -0800

    Merge branch 'master' into barugon/color_table

commit d2d0df4
Author: Barugon <barugon@dungeonbox.net>
Date:   Sat Feb 12 17:37:30 2022 -0800

    Merge branch 'master' into barugon/color_table

commit 4b91e3d
Author: Barugon <barugon@dungeonbox.net>
Date:   Mon Feb 7 05:27:15 2022 -0800

    Better casting in entry_count

commit df40642
Author: Barugon <barugon@dungeonbox.net>
Date:   Mon Feb 7 05:25:12 2022 -0800

    Have entry count return usize

    Make index usize for entry and entry_as_rgb

commit 7b3dbc2
Author: Barugon <barugon@dungeonbox.net>
Date:   Mon Jan 31 06:41:15 2022 -0800

    Add PR link

commit 293878e
Author: Barugon <barugon@dungeonbox.net>
Date:   Mon Jan 31 06:35:48 2022 -0800

    Use unreachable macro

commit 2b74b7e
Merge: 67423e5 8d347ba
Author: Barugon <barugon@dungeonbox.net>
Date:   Sun Jan 30 20:39:34 2022 -0800

    Merge branch 'master' into barugon/color_table

commit 67423e5
Author: Barugon <barugon@dungeonbox.net>
Date:   Sun Jan 30 20:31:52 2022 -0800

    Add enumerations for color types

commit 04b1b49
Author: Barugon <barugon@dungeonbox.net>
Date:   Thu Jan 27 23:01:31 2022 -0800

    Implement color table test

commit 3d9be87
Author: Barugon <barugon@dungeonbox.net>
Date:   Thu Jan 27 23:01:18 2022 -0800

    Add geo-tiff for color table testing

commit 0a1769d
Author: Barugon <barugon@dungeonbox.net>
Date:   Thu Jan 27 08:35:01 2022 -0800

    Add doc comment for ColorTable

    Remove raster_band method

commit 6c618b4
Author: Barugon <barugon@dungeonbox.net>
Date:   Tue Jan 25 15:59:42 2022 -0800

    Note changes in Unreleased section

commit 1e75ff3
Author: Barugon <barugon@dungeonbox.net>
Date:   Tue Jan 25 15:44:33 2022 -0800

    Implement ColorTable struct

    Add color_table method to RasterBand
@Barugon
Copy link
Contributor Author

Barugon commented May 13, 2022

@lnicola
I think this squash went better.

@lnicola
Copy link
Member

lnicola commented May 13, 2022

Thanks, I'll take a better look tomorrow.

lnicola
lnicola reviewed May 14, 2022
View reviewed changes
src/raster/rasterband.rs

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct GrayEntry {
pub g: i16,
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the i32 / usize discussion: do you know how often these happen to be negative? I mean, if I have a 16-bit RGB image, I'll still want positive numbers here.

Anyway, this isn't blocking, I was just wondering how GDAL expects these to be used.

Copy link
Contributor Author

@Barugon Barugon May 14, 2022 •
edited

Choose a reason for hiding this comment

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

I don't think they would ever be negative but I thought it might be best to leave them the same as what gdal_sys provides. I suppose elevation data could be negative but I can't imagine that ever being palettized.

lnicola
lnicola reviewed May 14, 2022
View reviewed changes
src/raster/rasterband.rs Outdated
pub struct ColorTable<'a> {
palette_interpretation: PaletteInterpretation,
c_color_table: GDALColorTableH,
_raster_band: &'a RasterBand<'a>,
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a PhantomData<&'a RasterBand<'a>> or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unaware of PhantomData. Thanks for pointing it out.

@lnicola
Copy link
Member

lnicola commented May 14, 2022

bors r=ChristianBeilschmidt,lnicola

@bors
Copy link
Contributor

bors bot commented May 14, 2022

Build succeeded:

@bors bors bot merged commit b700dc0 into georust:master May 14, 2022
@Barugon Barugon deleted the barugon/color_table branch May 14, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lnicola lnicola lnicola left review comments

@ChristianBeilschmidt ChristianBeilschmidt ChristianBeilschmidt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Barugon @ChristianBeilschmidt @jdroenner @lnicola